Skip to content

a couple of minor? adjustments - #7

Open
robertramey wants to merge 3 commits into
cierelabs:masterfrom
robertramey:master
Open

a couple of minor? adjustments#7
robertramey wants to merge 3 commits into
cierelabs:masterfrom
robertramey:master

Conversation

@robertramey

Copy link
Copy Markdown

I made a few changes in order to get this to build and work for my project on OSX with Clang

a) I had to tweak the CMake files to make things work with Xcode. I does build an Xcode project but I'm not sure that it's all correct. That is I don't think the Xcode tests work as expected. This is outside our scope - I think it's an issue regarding the CMake Xcode implementation. It does build the library however and that was enough for me. I seem to recall that I had some issues regarding the selection of static vs shared library build. It seems that the best setup is to build a static library which links to the shared library version of the standard libraries. This seems to contradict normal recommended practice of using all static or all shared libraries. Hopefully I made the right choice here - but I'll leave it to the experts. I'm hoping you don't think I'm the expert.

b) I need the parser to parse just part of the file. It turns out that it consumed one more character than necessary when used with an input stream iterator. I "fixed" it by doing an unmet after calling the parser. Again, I'm not sure is this really correct - I'll leave that to you.

Robert Ramey

tweaks to support input stream iterator usage
I found that the parser looks ahead one too many characters for my application.

Consider these as suggestions as I don't fully understand either of these two modules
tweaks to support input stream iterator usage
I found that the parser looks ahead one too many characters for my application.

Consider these as suggestions as I don't fully understand either of these two modules

And I'm not too hot on git either
@mjcaisse

Copy link
Copy Markdown
Member

Thank you Robert! We are taking a look at your changes.

Comment thread CMakeLists.txt

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope we can leave this one out

@jefftrull

Copy link
Copy Markdown
Contributor

Thanks for helping out with the code, Robert. I have two meta-comments:

  1. Let's take the .DS_Store commit out of there... I'm sure it was an accident, but it muddies the pull
  2. Also it looks like your changes are against an older version of json_spirit. Furthermore, I'm going to take a couple of your changes that I found uncontroversial and incorporate them in a new commit, and then either Joel or Jeroen will give it a try on their OSX boxes. So in a day or two we should have a new version for you to "pull" and merge with, after which you would ideally submit a new pull request with any remaining diffs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants