Skip to content

Don't use unique_ptr on pre-c++11 branch #238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from

Conversation

sbc100
Copy link
Contributor

@sbc100 sbc100 commented Apr 10, 2015

Don't use C++11 unique_ptr in the 0.y.z branch.
Although this usage is guarded with __cplusplus >= 201103
some build configurations (notably chromium) use a
C++11-compliant compiler but a pre-11 library.

cdunn2001 and others added 11 commits March 31, 2015 15:07
in case of embedded nulls
This partially reverts commit 45cd949.

Ignored ValueInternal* changes, since those did not produce symbols for
Debian build. (They must not have used the INTERNAL stuff.)

Ignored CZString changes since those are private (and sizeof struct did
not change).

  open-source-parsers#78

Conflicts:
	include/json/value.h
	src/lib_json/json_internalarray.inl
	src/lib_json/json_internalmap.inl
	src/lib_json/json_value.cpp
re: 28836b8

A global instance of a Value (viz. 'null') was a mistake,
but dropping it breaks binary-compatibility. So we will keep it
everywhere except the one platform where it was crashing, ARM.
…on non-standard JSON.'

revert '642befc836ac5093b528e7d8b4fd66b66735a98c',
but keep the *added* methods for `decodedNumber()` and `decodedDouble()`.
Don't use C++11 unique_ptr in the 0.y.z branch.
Although this usage is guarded with __cplusplus >= 201103
some build configurations (notably chromium) use a
C++11-compliant compiler but a pre-11 library.
@cdunn2001
Copy link
Contributor

I'm confused. How can unique_ptr not exist for C++11? This seems like a serious problem with Chromium. What do you means by "a pre-11 library"? Which library? jsoncpp? stdc++?

@sbc100
Copy link
Contributor Author

sbc100 commented Apr 11, 2015

It does sound strange doesn't it. Because chromium supports a lot of platforms, C++11 support across the codebase can only be fully supported once all the target platforms support C++11. The current status is here: https://chromium-cpp.appspot.com/. I think most of the C++11 language features are now allows but so far none of the library (libstdc++/libc++) features. To quote from that page "In particular, chromium/android is currently using STLport, and chromium/mac is currently using libstdc++4.2, which predate C++11.".

I figured since this branch was specifically for pre-C++11 builds that you might be OK with this change. I not, we can patch it in chromium. Or perhaps you would be OK with a different macro to guard this?

@cdunn2001
Copy link
Contributor

Ugh! That's maddening. I understand why they would want language features without the libraries -- to facility the migration to C++11 -- but they need to provide an extra macro.

Ok. We'll pull your commit on the 0.y.z branch. Do you need a release? Or is the branch enough?

cdunn2001 pushed a commit that referenced this pull request Apr 12, 2015
Don't use C++11 unique_ptr in the 0.y.z branch.
Although this usage is guarded with __cplusplus >= 201103
some build configurations (notably chromium) use a
C++11-compliant compiler but a pre-11 library.

pull #238
@sbc100
Copy link
Contributor Author

sbc100 commented Apr 12, 2015

Branch commit is fine. We will pin to a sha1.

@cdunn2001 cdunn2001 closed this Apr 12, 2015
@cdunn2001 cdunn2001 assigned cdunn2001 and unassigned cdunn2001 Apr 12, 2015
@sbc100
Copy link
Contributor Author

sbc100 commented Apr 13, 2015

Hmm, strange, did you end up re-writing history on the 0.y.z branch? Was that deliberate? I makes things difficult for existing forks when that happens.

@sbc100
Copy link
Contributor Author

sbc100 commented Apr 13, 2015

I see it looks like for each 0.10.X release you are doing a rebase onto master (is that right?). Would it be possible to instead merge master into 0.y.x each time to avoid re-writing history in the future?

@cdunn2001
Copy link
Contributor

The 0.y.z branch is continually rebased to master. Yes, merging would help forks, but my biggest trouble is maintenance of multiple branches. By rebasing, I make it completely obvious what the patches are. I don't really want forks of the maintenance branch.

I'd rather not start merging. It creates too much of a mess. But I could instead cherry-pick changes from 1.y.z to 0.y.z, assuming the changes are small. In the past, we've had significant re-formatting, esp. when we added clang support.

Let me think about this...

My first thought is that you could base your fork branch on the master branch. Just once, cherry-pick the extra patches from the 0.y.z branch. And in the future, you can merge from master into your own branch, whenever you choose. What do you think of that idea?

@sbc100
Copy link
Contributor Author

sbc100 commented Apr 13, 2015

I agree that its nicer to have a linear history for your 0.y.z branch that always sits nicely on top of your master branch. However, I don't think history re-writing is ever a good idea in a public repo. I had trouble with my fork locally and it also caused the chromium mirror of jsoncpp (https://chromium.googlesource.com/external/github.com/open-source-parsers/jsoncpp/) to stop working (this is probably fairly chrome specific since the mirroring code doesn't allow for 'push -f'). My advise is to live with merging when the branch is public.

The other downside to merging is that it makes comparing 0.10.1 and 0.10.2 hard. You end up seeing the all the commits twice: git l 0.10.1..0.10.2 should only show one or two commits but it shows all of them since the two tags we effectively made on different branches.

Also, even with merging it should be very obvious what the patches are: git log master..0.y.z will always show you exactly what changes are on the branch.

Just advice. I don't mean to tell you how to run your project.

@cdunn2001
Copy link
Contributor

I hadn't considered 0.y.z a "public" branch, but if that's how Chromium uses it, fine. Let's go with that.

I need a rebase-able branch, but it doesn't have to be the one releases come from. So I'll start a new branch, private-0.y.z, and I can ensure that the public, merged branch is always identical. That would work for me.

How far back do you need to go? Re-create 0.10.2 from 0.10.1 by merging? Or just start merging in the future?

@sbc100
Copy link
Contributor Author

sbc100 commented Apr 13, 2015

Just for the future would be great. Thanks!

cdunn2001 pushed a commit to cdunn2001/jsoncpp that referenced this pull request Apr 19, 2015
Don't use C++11 unique_ptr in the 0.y.z branch.
Although this usage is guarded with __cplusplus >= 201103
some build configurations (notably chromium) use a
C++11-compliant compiler but a pre-11 library.

pull open-source-parsers#238
cdunn2001 pushed a commit to cdunn2001/jsoncpp that referenced this pull request Jun 19, 2015
Don't use C++11 unique_ptr in the 0.y.z branch.
Although this usage is guarded with __cplusplus >= 201103
some build configurations (notably chromium) use a
C++11-compliant compiler but a pre-11 library.

pull open-source-parsers#238
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.

2 participants