remove uthash #733
Closed
remove uthash #733
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
requested review from
rajachan,
bwbarrett,
rauteric and
a team
as code owners
|
xxx: I don't really want the commit that moves the unit tests to C++, but due to the stupid "libinternal" library we have, it's easier to just do this than debug autotools/libtool issues. |
added 5 commits
December 3, 2024 08:50
g++7 fails to initialize C-style tagged unions with the following error message: > sorry, unimplemented: non-trivial designated initializers not > supported Note that gcc7 handles this fine when the file is parsed as C, as well as g++8 and beyond, and the problem only exists when operating in C++ mode under g++7. AL2 is still using this ancient toolchain, so add a hack in the case that it is used. This is pretty horrible, and should be cleaned up sooner rather than later. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
bad74fc re-triggered -Wnarrowing on rebase. The natural type of the array rvalue here is size_t[] because it's defined in terms of the stripe size, and the stripe size array is size_t[]. The entire rval is then implicitly converted to int[] through assignment, which causes narrowing. Offsets here should just be size_t in the first place, though, as it's what the uut is expecting. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
On builds where lttng is enabled, the inclusion of nccl_ofi_tracepoint.h pulls in <lttng/ust-utils.h>, which defines templates when __cplusplus is defined without restraint. Those templates cannot have C linkage. This file does not really need nccl_ofi_tracepoint.h; it only needs config.h to test for whether NVTX tracing is enabled, and then it needs nvtx3 headers in the case that it is so that it may define storage for the nvtx domain handles. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
All included headers are fully safe to be parsed as C++, so these unit tests can now use C++. Rename them to `.cc' files and use a few c++ keywords to enforce that the compiler is actually treating them as such. Actual C++ cleanups to follow later, hopefully alongside a unit test framework like gtest or catch2. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
ghost
force-pushed
the
uthash-removal
branch
3 times, most recently
from
39bf7b9 to
70a4020
Compare
implement a small wrapper around std::map, use this instead of uthash. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
|
apologies on the pushes, a few straggler header changes ended up on the wrong commits. Believe this is done now. |
added 2 commits
December 3, 2024 11:33
move rdma.c to c++, move some structs from the header to the unit so that it can use c++ types. Implement ep_addr_list with unordered_map/unordered_set instead of uthashisms. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
there are no remaining consumers of uthash or ep_addr_list after this commits parent. Delete code. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
bwbarrett
requested changes
Dec 3, 2024
| * Endpoint rail encapsulates data of an endpoint for a | ||
| * specific rail. | ||
| */ | ||
| struct nccl_net_ofi_ep_rail { |
There was a problem hiding this comment.
I don't think we should make this change. We want to split up rdma.c, not make it harder to split up later.
There was a problem hiding this comment.
Even in light of the conversation below, I still have strong feelings on this one. This is not a useful change for us.
There was a problem hiding this comment.
Can we put the ep map code into a new file nccl_ofi_rdma_ep_map.{h,c} or some such?
Closed
ghost
closed this
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
(builds atop two other branches)
we had two consumers of uthash:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.