Cppfixes 201704 #309

msjarrett · 2017-04-06T22:34:11Z

Please consider the following changes - these fix build issues in our environment, and perhaps will help others.

Adding :: to djinni namespace inside the DJINNI_ASSERT macros. This is already done in other macros in the file, and prevents a deeper namespace from being chosen inadvertently.
proxy_cache_interface.hpp uses shared_ptr in a template class without including .
Something about objectFromHandleAddress() is causing the compiler to segfault on the specified line. Not sure why (still investigating), but in the meantime, the temporary variable allows the compiler to complete.

Thanks!

- Adding global namespace scope to DJINNI_ASSERT to prevent conflict with local namespaces. - Add <memory> include to reference shared_ptr. - Use temporary variable in djinni_support.hpp to avoid compiler segfault.

smarx · 2017-04-06T22:34:13Z

Automated message from Dropbox CLA bot

@msjarrett, it looks like you've already signed the Dropbox CLA. Thanks!

j4cbo · 2017-04-06T23:49:02Z

support-lib/jni/djinni_support.hpp

- const auto & ret = reinterpret_cast<const CppProxyHandle<T> *>(handle)->get();
+ // Below line segfaults gcc-4.8. Using a temporary variable hides the bug.
+ //const auto & ret = reinterpret_cast<const CppProxyHandle<T> *>(handle)->get();
+ const CppProxyHandle<T> *temp = reinterpret_cast<const CppProxyHandle<T> *>(handle);


Name this "proxy_handle" or something more descriptive?

artwyman

Thanks for the contributions. A few comments on individual lines, but looks good overall.

artwyman · 2017-04-07T00:18:15Z

README.md

@@ -437,6 +437,9 @@ Run `make test` to invoke the test suite, found in the test-suite subdirectory.
 - Iulia Tamas
 - Andrew Twyman

+## Contributors


@msjarrett can you explain this addition please? I assume you're at Google, so I'd guess this isn't inaccurate, but it's far from complete since we haven't made a habit of calling out all individual people/companies which contribute in the past.

Our OSS policy asks us to add the company to the list of contributors, but as you mentioned, there isn't really a complete list in this project. The readme seems like the wrong place, but it'd be even weirder for me to create a new authors file for it.

Any suggestion for a better place for this?

Hmm, yeah, maybe we should think about having an official list. This is the first time someone's raised the issue of a policy like this. Is the policy "you must add to the list" or can it be interpreted as "add to the list if it exists"? It seems like in our current state our "contributors list" is git log, and I don't know if there are global norms on GitHub about doing otherwise.

Is this coming from this policy: https://opensource.google.com/docs/patching/#how-to-patch

If so, that seems to suggest "If copyright authors are listed in a LICENSE, COPYING, AUTHORS", which wouldn't apply here. We could work on changing that if you'd like, but may not need to delay this diff on it.

It may be that the current list of "Authors" is poorly labeled/named. It's a list of "primary" contributors, not all contributors. We've used it as a way to recognize people who wrote large chunks of the tool, or whole new features. It's not intended to be a claim of copyright, since the whole project is copyrighted by Dropbox, per the CLA.

Yes, the above link is the basis of the policy. I can see how it could be interpreted either way, but my compliance team is insisting it applies for this particular change.
Is there somewhere I could put this that would be more suitable?

Hmm, the consensus in the Dropbox open-source team is the opposite. "If copyright authors are listed in a LICENSE, COPYING, AUTHORS or similar file" evaluates to false, so no action is required. The only place our contributors are listed is in git history, which will be attributed when I merge this. Can you possibly put your compliance team in touch with us? Do you know if it's the Authors list which is at issue here, or do they think the addition is required even if it's the first one.

Anyhow, I'd rather not let legal wrangling actually get in the way of a good contribution. I'd be open to possibly adding a CONTRIBUTORS file, with a paragraph at the top explaining it's not intended to be complete (the complete list being the git history), but only present where required for legal purposes. I'd also be open to clarifying/renaming the Authors section in the README to make it clear it's a way of acknowledging "top contributors" not a list of all contributors.

Swapped the README.md change for a CONTRIBUTORS file. Does this work?

artwyman

All the code looks good to me. Merge at this point is only pending the discussion on contribution credit for Google, Inc.

msjarrett · 2017-04-13T18:26:49Z

Changed the README.md for a CONTRIBUTORS file.

msjarrett · 2017-04-20T17:39:07Z

@artwyman Does the contributors change address your concern?

msjarrett · 2017-05-02T22:32:06Z

Anybody....?

artwyman · 2017-06-10T02:53:34Z

Hi @msjarrett. Sorry for the long delay. I didn't intentionally stonewall this. It just came in at around the same time as I got too busy to keep up with Djinni for a while. I don't love the CONTRIBUTORS file, but I'm willing to live with it to keep the lawyers happy. Will merge shortly.

msjarrett added 2 commits April 6, 2017 10:30

Making three changes to fix compilation in our environment.

eb02361

- Adding global namespace scope to DJINNI_ASSERT to prevent conflict with local namespaces. - Add <memory> include to reference shared_ptr. - Use temporary variable in djinni_support.hpp to avoid compiler segfault.

Adding comment to warn of the potential segfault in gcc.

6a3b969

j4cbo reviewed Apr 6, 2017

View reviewed changes

artwyman suggested changes Apr 7, 2017

View reviewed changes

artwyman added the bug label Apr 7, 2017

Renaming "temp" to "proxy_handle" to make more readable.

bdbdcc0

artwyman suggested changes Apr 7, 2017

View reviewed changes

Swap the README.md change for a CONTRIBUTORS file for attribution.

c73950e

artwyman merged commit bca8b4b into dropbox:master Jun 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cppfixes 201704 #309

Cppfixes 201704 #309

msjarrett commented Apr 6, 2017

smarx commented Apr 6, 2017

j4cbo Apr 6, 2017

msjarrett Apr 7, 2017

artwyman left a comment

artwyman Apr 7, 2017

msjarrett Apr 7, 2017

artwyman Apr 7, 2017

artwyman Apr 7, 2017

msjarrett Apr 7, 2017

artwyman Apr 7, 2017

msjarrett Apr 11, 2017

artwyman left a comment

msjarrett commented Apr 13, 2017

msjarrett commented Apr 20, 2017

msjarrett commented May 2, 2017

artwyman commented Jun 10, 2017

Cppfixes 201704 #309

Cppfixes 201704 #309

Conversation

msjarrett commented Apr 6, 2017

smarx commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman left a comment

Choose a reason for hiding this comment

msjarrett commented Apr 13, 2017

msjarrett commented Apr 20, 2017

msjarrett commented May 2, 2017

artwyman commented Jun 10, 2017