-
Notifications
You must be signed in to change notification settings - Fork 86
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
fix: add inline to prevent multiple definitions #2606
Conversation
@ianna I notice that it gives an error - |
@@ -19,7 +19,7 @@ namespace awkward { | |||
|
|||
/// @brief Object of {@link BuilderOptions BuilderOptions} which sets the | |||
/// values of the default options. | |||
awkward::BuilderOptions default_options(1024, 1); | |||
inline awkward::BuilderOptions default_options(1024, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an inlined variable - does it need to be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we include LayoutBuilder.h
in multiple source files, it results in multiple definitions of default_options
leading to linker errors. So yes it needs to be inline.
test-fake.cpp:(.text+0x360): multiple definition of `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const awkward::type_to_numpy_like<long>()'; /tmp/ccmHoZHl.o:fake.cpp:(.text+0x360): first defined here
/usr/bin/ld: /tmp/cculuiVU.o:(.bss+0x0): multiple definition of `awkward::LayoutBuilder::default_options'; /tmp/ccmHoZHl.o:(.bss+0x0): first defined here
@jpivarski
|
Ah, right! I missed that If you want a quick-and-dirty way to ensure that this PR does not depend on #2608, you could replace the inline variable with a preprocessor #define AWKWARD_LAYOUTBUILDER_DEFAULT_OPTIONS awkward::BuilderOptions(1024, 1) and then use For completeness, all of the functions ought to be
In utils.h, I see:
|
Yes, this can be a temporary workaround until the version isn't updated to C++17. Should I add these changes to this PR?
|
Okay, then it sounds good. Let's use the preprocessor as a temporary workaround for not requiring C++17 yet. (Our next opportunity to do that is September 1.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ManasviGoyal! Looks good to me.
@@ -531,7 +532,8 @@ test_ListOffset_Empty() { | |||
"\"class\": \"ListOffsetArray\", " | |||
"\"offsets\": \"i64\", " | |||
"\"content\": { " | |||
"\"class\": \"EmptyArray\" " | |||
"\"class\": \"EmptyArray\", " | |||
"\"parameters\": {} " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManasviGoyal why does this test change? Was it not being run before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run the the LayoutBuilder
tests, the assertion is failing due to the missing "parameters: {}"
described in the EmptyArray
form. If this test is run while running all the tests, it's strange that it doesn't give any error despite the incomplete form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the Forms should explicitly require a parameters
key when reading from JSON. The parameters
key is always extracted with get
:
awkward/src/awkward/forms/form.py
Lines 36 to 42 in 089f843
def from_dict(input: Mapping) -> Form: | |
assert input is not None | |
if isinstance(input, str): | |
return ak.forms.NumpyForm(primitive=input) | |
assert isinstance(input, Mapping) | |
parameters = input.get("parameters", None) |
(ak.forms.from_json
immediately calls ak.forms.from_dict
. When parameters
are None
, it's the same as if they're {}
.)
On top of that, EmptyArray is required to not have any parameters
. Explicitly providing None
or {}
is benign, though:
awkward/src/awkward/forms/emptyform.py
Lines 15 to 21 in 089f843
class EmptyForm(Form): | |
is_numpy = True | |
is_unknown = True | |
def __init__(self, *, parameters: JSONMapping | None = None, form_key=None): | |
if not (parameters is None or len(parameters) == 0): | |
raise TypeError(f"{type(self).__name__} cannot contain parameters") |
If these are C++ tests that are failing, and the C++ tests are more restrictive than what the Python tests require, then it's the test that should change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C++ tests were failing because of the mismatch between the EmptyArray
form definition in the code and the tests. If the parameters are not needed in this case, then the parameters part from the EmptyArray
form in the Layoutbuilder.h
code can be removed then we don't have to change the tests
awkward/header-only/layout-builder/awkward/LayoutBuilder.h
Lines 416 to 419 in aba0b8d
std::string | |
form() const noexcept { | |
return "{ \"class\": \"EmptyArray\", \"parameters\": {} }"; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. It's also the only node type that sets it to a constant; all of the other LayoutBuilder node types take a parameters
argument to pass on here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. I'll update the PR with the required changes.
The following changes are made -
default_options
,type_to_name
, andtype_to_numpy_like
.#define
is used as a temporary workaround in place ofdefault_options
as inline variables are supported starting from C++17. Once the requirements are updated to C++17,default_options
can be inlined.test_1494-layout-builder.cpp
which included the EmptyArray case were failing due to missing parameter string in the form, so that has been fixed too by removing parameters from theLayoutBuilder
code.