This is an archive of the discontinued LLVM Phabricator instance.

Print nullptr_t namespace qualified within std::
ClosedPublic

Authored by dblaikie on Sep 19 2021, 2:34 PM.

Details

Summary

This improves diagnostic (& important to me, DWARF) accuracy - otherwise
there could be ambiguities between "std::nullptr_t" and some user-defined
type that's /actually/ "nullptr_t" defined in the global namespace.

Diff Detail

Event Timeline

dblaikie requested review of this revision.Sep 19 2021, 2:34 PM
dblaikie created this revision.
Herald added a project: Restricted Project. · View Herald Transcript

This improves diagnostic (& important to me, DWARF) accuracy

FWIW, I don't think the diagnostic particularly needs more accuracy here -- I think users know what nullptr_t type is being referred to without the full qualification because of other contextual clues in the diagnostic text. However, I'm not opposed to the changes as I don't think they make anything worse. But I didn't see any tests for DWARF behavioral changes, are those changes in a future patch, or should there be some more test coverage?

clang/lib/AST/Type.cpp
3045

Should this be ::std::nullptr_t to differentiate it from odd things like:

namespace my {
namespace std {
class nullptr_t {};
}
}

This improves diagnostic (& important to me, DWARF) accuracy

FWIW, I don't think the diagnostic particularly needs more accuracy here -- I think users know what nullptr_t type is being referred to without the full qualification because of other contextual clues in the diagnostic text. However, I'm not opposed to the changes as I don't think they make anything worse. But I didn't see any tests for DWARF behavioral changes, are those changes in a future patch, or should there be some more test coverage?

Oh, happy to add a dedicated DWARF test, but hadn't originally planned on it - since it relies on all the same type printing infrastructure anyway. I'll see about adding one.

clang/lib/AST/Type.cpp
3045

I was hoping not to get overly pedantic - I think clang omits the global namespace scope when naming other types in namespaces?

Yeah:

scope.cpp:5:5: error: invalid operands to binary expression ('int' and 'ns::inner')
  1 + ns::inner();
  ~ ^ ~~~~~~~~~~~

So this seems consistent with that, at least. That's true also when rendering template parameter type names, etc, so far as I know.

dblaikie updated this revision to Diff 373768.Sep 20 2021, 7:07 PM

Add debug info test case

aaron.ballman accepted this revision.Sep 21 2021, 4:37 AM

LGTM!

clang/lib/AST/Type.cpp
3045

Okay, I'm sold, thank you!

This revision is now accepted and ready to land.Sep 21 2021, 4:37 AM
This revision was landed with ongoing or failed builds.Sep 21 2021, 11:22 AM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Sep 21 2021, 11:23 AM
clang/lib/AST/Type.cpp
3045

Thanks for the review!