This is an archive of the discontinued LLVM Phabricator instance.

[clang][DR2621] using enum NAME lookup fix
ClosedPublic

Authored by urnathan on Sep 20 2022, 6:54 AM.

Details

Reviewers
aaron.ballman
Group Reviewers
Restricted Project
Commits
rG3d2080683f1d: [clang][DR2621] using enum NAME lookup fix
Summary

Although using-enum's grammar is 'using elaborated-enum-specifier',
the lookup for the enum is ordinary lookup (and not the tagged-type
lookup that normally occurs wth an tagged-type specifier). Thus (a)
we can find typedefs and (b) do not find enum tags hidden by a non-tag
name (the struct stat thing).

This reimplements that part of using-enum handling, to address DR2621,
where clang's behaviour does not match std intent (and other
compilers).

Diff Detail

Event Timeline

urnathan created this revision.Sep 20 2022, 6:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 6:54 AM
urnathan requested review of this revision.Sep 20 2022, 6:54 AM

Thank you for working on this! The precommit CI failures on Windows look to be unrelated. Btw, when you generate the patch, if you could add some more context to the patch (with -U9999) that would be appreciated -- it makes it easier to review when we can see the surrounding code as well.

You should add a release note for the changes as well. It looks like we've not updated the C++ DR status page to have the latest round of DRs, so no need to regenerate that page. But you should still add a test case to clang/test/CXX/drs/dr26xx.cpp with the appropriate markup (see other examples in that file/test directory) so that we get the correct status when we do go to regenerate that page.

clang/include/clang/Basic/DiagnosticSemaKinds.td
607–608

I don't think we need this one; we can use err_unknown_typename.

clang/lib/Parse/ParseDeclCXX.cpp
722–723

Spell out the types because they're not spelled out in the initialization.

clang/lib/Sema/SemaDeclCXX.cpp
11852

I think you should consider using ClassifyName() instead, as that will not only get you the parsed type information, but it handles things like typo correction, etc. Then you shouldn't need to worry about the diagnostics immediately below.

11862

These days we use dyn_cast_if_present instead.

urnathan updated this revision to Diff 462160.Sep 22 2022, 6:16 AM
urnathan marked 3 inline comments as done.

I forgot I had a git abbriev for the -U9999 thing, sorry about that.

I tried ClassifyName, but although it claimed to have a type, its getType method returned a ParsedType object that seemed broken -- its 'get' methof produced a thing not recognized as valid QualType and whose dump method seg faulted when called.

urnathan added inline comments.Sep 22 2022, 6:17 AM
clang/lib/Sema/SemaDeclCXX.cpp
11852

See comments, tried & failed.

I forgot I had a git abbriev for the -U9999 thing, sorry about that.

No worries!

I tried ClassifyName, but although it claimed to have a type, its getType method returned a ParsedType object that seemed broken -- its 'get' methof produced a thing not recognized as valid QualType and whose dump method seg faulted when called.

Wha? That seems rather concerning. What does getKind() return? (We assert that the kind is correct when trying to get the data out of the object, but perhaps you weren't using an asserts build?)

Wha? That seems rather concerning. What does getKind() return? (We assert that the kind is correct when trying to get the data out of the object, but perhaps you weren't using an asserts build?)

getKind() says it's a type and getType() returns a thing that could be a (nonnull) ParsedType. That objects get call returns a QualType I am unable to inspect, and that got printed out in diagnostics as no characters (when it failed the getAsTaggedType call). I find the wrapped pointer objects clang seems to love hard to inspect. I tried tracing through ClassifyName to see how it was constructing the type, but got somewhat lost in a set of lambdas. FWIW ClassifyName seems to be mostly employed in figuring out tentative parse disambiguation.

I am using unoptimized assert builds :)

Wha? That seems rather concerning. What does getKind() return? (We assert that the kind is correct when trying to get the data out of the object, but perhaps you weren't using an asserts build?)

getKind() says it's a type and getType() returns a thing that could be a (nonnull) ParsedType. That objects get call returns a QualType I am unable to inspect, and that got printed out in diagnostics as no characters (when it failed the getAsTaggedType call). I find the wrapped pointer objects clang seems to love hard to inspect. I tried tracing through ClassifyName to see how it was constructing the type, but got somewhat lost in a set of lambdas. FWIW ClassifyName seems to be mostly employed in figuring out tentative parse disambiguation.

I am using unoptimized assert builds :)

Well huh, TIL! :-D

I'm happy with the state of things, so LGTM, but I did spot the need for a comment to be added to a test case that you should fix when landing, so the DR scraping script works properly.

clang/test/CXX/drs/dr26xx.cpp
3
This revision is now accepted and ready to land.Sep 23 2022, 5:57 AM
junaire added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
725

minor suggestion: will it be easier to read if you add comments like /*ParsedType=*/nullptr, /*ObjectHasErrors=*/false and etc?

shafik added a subscriber: shafik.Sep 23 2022, 3:33 PM
shafik added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
725

We should be doing this it is part of the style guide.

This can also be verified via clang-tidy bugprone-argument-comment

Seems DR2621 is not yet listed in https://github.com/llvm/llvm-project/blob/main/clang/www/cxx_dr_status.html (but if it were, it should be updated...)

urnathan updated this revision to Diff 462951.EditedSep 26 2022, 9:53 AM
urnathan marked 4 inline comments as done.

Here's an update, just to check I've got all the comments addressed. It doesn't appear this DR has made it to the ready status in cwg-index, so ./make_cxx_dr_status barfed.

(For avoidance of doubt, I was at the CWG mtg last week when we discussed this and moved it to Ready).

ETA: I'll commit in a few days, if there are not further comments.

aaron.ballman accepted this revision.Sep 27 2022, 4:31 AM

Reaffirming my LG but there was a minor formatting thing for the release notes that you can fix when landing. Thank you! (Btw, I was also at that CWG and can confirm this matches the resolution of the DR as discussed.)

clang/docs/ReleaseNotes.rst
359
urnathan marked an inline comment as done.Sep 28 2022, 5:06 AM
This revision was landed with ongoing or failed builds.Sep 28 2022, 8:50 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 8:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript