Page MenuHomePhabricator

Fix for Bug 14644 - clang confuses scope operator for global namespace giving extra qualification on member
Needs ReviewPublic

Authored by ryee88 on Feb 7 2016, 8:15 AM.

Details

Summary

Bug 14644 - clang confuses scope operator for global namespace giving extra qualification on member

This is a fix for this bug: https://llvm.org/bugs/show_bug.cgi?id=14644

Essentially the diagnostic needs to distinguish between a "global-specifier with no nested-name-specifier" and nested-name-specifier (everything else) in order to provide a more helpful error message.

Diff Detail

Event Timeline

ryee88 updated this revision to Diff 47133.Feb 7 2016, 8:15 AM
ryee88 retitled this revision from to Fix for Bug 14644 - clang confuses scope operator for global namespace giving extra qualification on member.
ryee88 updated this object.
ryee88 added a subscriber: cfe-commits.

Ping-- I think this review got missed.

ryee88 updated this revision to Diff 47922.Feb 13 2016, 6:37 PM

Added unit tests.

ryee88 updated this revision to Diff 47926.Feb 13 2016, 8:37 PM

Messed up the diff. Uploading it again.

ryee88 updated this revision to Diff 49866.Mar 4 2016, 5:56 PM
ryee88 added a subscriber: dblaikie.

Anyone care to take a look?

Anyone want to pick up this review?

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
lib/Sema/SemaDecl.cpp
3803 ↗(On Diff #49866)

Can drop the N-paper reference, and replace "3.4.4, section 3" with [basic.lookup.elab]p3

3804 ↗(On Diff #49866)

Ah, you're referring to text in the example; I think the reference above should actually be to [dcl.type.elab]

3805 ↗(On Diff #49866)

Can drop the clang::

3811 ↗(On Diff #49866)

Should remove the diagId = from the else clause.

Not sure I fully understand the issue with the existing diagnostic/change in wording.

'::' is a nested name specifier, even if it's a degenerate case, I think - so the old wording doesn't seem wrong, as such (& the new text seems correct in the other cases too - should we just switch to that in general rather than having two cases?)

ryee88 added inline comments.Mar 18 2016, 5:51 AM
lib/Sema/SemaDecl.cpp
3803 ↗(On Diff #49866)

I'm going to re-organize these comments and move the conditional because really all these comments apply too.

3805 ↗(On Diff #49866)

ok

3811 ↗(On Diff #49866)

whoops. will do.

Not sure I fully understand the issue with the existing diagnostic/change in wording.

'::' is a nested name specifier, even if it's a degenerate case, I think - so the old wording doesn't seem wrong, as such (& the new text seems correct in the other cases too - should we just switch to that in general rather than having two cases?)

Yes that's the basically the issue raised in the bug report. The error wording for the degenerate nested name specifier is unclear. I'll tinker around with a generic wording.

rsmith edited edge metadata.Mar 18 2016, 6:31 AM

Note that the diagnostic wording has already been fixed compared with the diagnostic reported in that bug report. Do we actually need more changes here?

Yeah I don't think it's worth it to modify the general case wording to to explicitly call out the degenerate global nested-name-specifier. It'll cause more confusion for the general case.

At this point, do we just close the bug and keep the new unit tests?

ryee88 updated this revision to Diff 55707.Apr 29 2016, 9:02 PM
ryee88 edited edge metadata.

Removed unnecessary code changes. This patch contains a unit test to detect a regression of this global name specifier issue.

gribozavr resigned from this revision.Feb 13 2019, 5:48 AM