This is an archive of the discontinued LLVM Phabricator instance.

Uglifiy 'internal' namespace
AbandonedPublic

Authored by rodgert on Mar 10 2019, 2:18 PM.

Details

Reviewers
ldionne

Event Timeline

rodgert created this revision.Mar 10 2019, 2:18 PM
ldionne requested changes to this revision.Mar 12 2019, 7:02 AM
ldionne added inline comments.
include/pstl/internal/algorithm_impl.h
68

Those calls should stay qualified. If a user had a function called __parallel_or and _ForwardIterator were a custom type, we could end up calling their function because of ADL.

I know __parallel_or is a reserved name, but I like being consistent especially when it comes to ADL. WDYT?

This revision now requires changes to proceed.Mar 12 2019, 7:02 AM
rodgert added a comment.EditedMar 12 2019, 9:47 AM

@ldionne - When Intel added multiple new algorithm implementations, they did so without qualifying these calls. This led to a situation where some were and some weren't qualified.

I chose to make them uniform during the uglification pass, I have a subsequent patch that I'm working on that adds back the namespace qualifications uniformly.
I would like to see this patch accepted as is, because that new patch relies on this as a baseline, and it doesn't really change the status quo where we have many calls that are not qualified in the code.

"I know __parallel_or is a reserved name, but I like being consistent especially when it comes to ADL. WDYT?"

According to @rsmith and @jwakely it's not just about consistency -

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60497

So we need to have it and have it applied uniformly.

ldionne accepted this revision.Mar 14 2019, 7:29 AM

@ldionne - When Intel added multiple new algorithm implementations, they did so without qualifying these calls. This led to a situation where some were and some weren't qualified.

I chose to make them uniform during the uglification pass, I have a subsequent patch that I'm working on that adds back the namespace qualifications uniformly.
I would like to see this patch accepted as is, because that new patch relies on this as a baseline, and it doesn't really change the status quo where we have many calls that are not qualified in the code.

"I know __parallel_or is a reserved name, but I like being consistent especially when it comes to ADL. WDYT?"

According to @rsmith and @jwakely it's not just about consistency -

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60497

So we need to have it and have it applied uniformly.

SGTM

This revision is now accepted and ready to land.Mar 14 2019, 7:29 AM
rodgert abandoned this revision.Mar 28 2019, 3:54 PM

Committed by other, non-phab, means