Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Cppfixes 201704 #309

Merged
merged 4 commits into from
Jun 10, 2017
Merged

Cppfixes 201704 #309

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb02361
Making three changes to fix compilation in our environment.
msjarrett Apr 6, 2017
6a3b969
Adding comment to warn of the potential segfault in gcc.
msjarrett Apr 6, 2017
bdbdcc0
Renaming "temp" to "proxy_handle" to make more readable.
msjarrett Apr 7, 2017
c73950e
Swap the README.md change for a CONTRIBUTORS file for attribution.
msjarrett Apr 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,9 @@ Run `make test` to invoke the test suite, found in the test-suite subdirectory.
- Iulia Tamas
- Andrew Twyman

## Contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

- Google Inc.

## Contacts
- Andrew Twyman - `[email protected]`
- Jacob Potter - `[email protected]`
11 changes: 7 additions & 4 deletions support-lib/jni/djinni_support.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,11 @@ void jniThrowAssertionError(JNIEnv * env, const char * file, int line, const cha

#define DJINNI_ASSERT_MSG(check, env, message) \
do { \
djinni::jniExceptionCheck(env); \
::djinni::jniExceptionCheck(env); \
const bool check__res = bool(check); \
djinni::jniExceptionCheck(env); \
::djinni::jniExceptionCheck(env); \
if (!check__res) { \
djinni::jniThrowAssertionError(env, __FILE__, __LINE__, message); \
::djinni::jniThrowAssertionError(env, __FILE__, __LINE__, message); \
} \
} while(false)
#define DJINNI_ASSERT(check, env) DJINNI_ASSERT_MSG(check, env, #check)
Expand Down Expand Up @@ -340,7 +340,10 @@ template <class T>
static const std::shared_ptr<T> & objectFromHandleAddress(jlong handle) {
assert(handle);
assert(handle > 4096);
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Name this "proxy_handle" or something more descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

const auto & ret = temp->get();
assert(ret);
return ret;
}
Expand Down
1 change: 1 addition & 0 deletions support-lib/proxy_cache_interface.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#pragma once

#include <memory>
#include <functional>
#include <typeindex>

Expand Down