[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. @rsmith - would it make sense to disable preferred names use when `PrintingPolicy::PrintCanonicalTypes` is used? It's used in `CGDebugInfo`, but also in `Sema::findFailedBooleanCondition` - so maybe we need another `PrintingPolicy` flag for this so `CGDebugInfo` can us

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. Oh, I should say I wasn't able to get this behavior to be exposed in a diagnostic in my limited testing - and attempting to add diagnostic cases caused me to lose the reproduction in the ast dumping, perhaps because I added some AST that tickled/fixed the disparity som

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. While looking into some debug info issues* I've come across what I think are some inconsistencies in the handling of this attribute - @rsmith mind taking a look? Firstly, it looks like something isn't handled when it comes to templates where only the declaration of a

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith added inline comments. Comment at: clang/include/clang/Basic/Attr.td:2384 + let Spellings = [Clang<"preferred_name", /*AllowInC*/0>]; + let Subjects = SubjectList<[ClassTmpl]>; + let Args = [TypeArgument<"TypedefType">]; jyknight wrote: > I wonder if th

[PATCH] D91311: Add new 'preferred_name' attribute.

jyknight added inline comments. Comment at: clang/include/clang/Basic/Attr.td:2384 + let Spellings = [Clang<"preferred_name", /*AllowInC*/0>]; + let Subjects = SubjectList<[ClassTmpl]>; + let Args = [TypeArgument<"TypedefType">]; I wonder if this attribute oug

[PATCH] D91311: Add new 'preferred_name' attribute.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG98f76adf4e94: Add new 'preferred_name' attribute. (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D91311?vs=307496&id=3099

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne added a comment. In D91311#2423809 , @rsmith wrote: > In D91311#2418881 , @ldionne wrote: > >> In D91311#2417293 , @rsmith wrote: >> >>> In D91311#2416940

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith added a comment. In D91311#2418881 , @ldionne wrote: > In D91311#2417293 , @rsmith wrote: > >> In D91311#2416940 , @ldionne wrote: >> >>> LGTM from the libc++ point of

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne added a comment. In D91311#2417293 , @rsmith wrote: > In D91311#2416940 , @ldionne wrote: > >> LGTM from the libc++ point of view. The CI is passing -- those failures are >> flaky modules tests that we need

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith added a comment. In D91311#2416940 , @ldionne wrote: > LGTM from the libc++ point of view. The CI is passing -- those failures are > flaky modules tests that we need to fix. Perhaps we need to specify `-fmodules-validate-system-headers` in the tes

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne accepted this revision as: libc++. ldionne added a comment. LGTM from the libc++ point of view. The CI is passing -- those failures are flaky modules tests that we need to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91311/new/ https

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith updated this revision to Diff 307496. rsmith added a comment. - Remove _LIBCPP_HAS_NO_PREFERRED_NAME Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91311/new/ https://reviews.llvm.org/D91311 Files: clang/include/clang/Basic/Attr.td clang

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne added a comment. In D91311#2404098 , @rsmith wrote: > In D91311#2403805 , @ldionne wrote: > >> We can stick with this design, but I'd like to understand why `#if >> _LIBCPP_HAS_NO_PREFERRED_NAME` is necessa

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith added a comment. In D91311#2403805 , @ldionne wrote: > We can stick with this design, but I'd like to understand why `#if > _LIBCPP_HAS_NO_PREFERRED_NAME` is necessary in ``, and also the CI is > failing on MacOS. You mean the HWAddressSanitizer

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne requested changes to this revision. ldionne added a comment. This revision now requires changes to proceed. Actually, apologies -- I might have accepted this too quickly. We can stick with this design, but I'd like to understand why `#if _LIBCPP_HAS_NO_PREFERRED_NAME` is necessary in ``,

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne accepted this revision as: libc++. ldionne added a comment. This revision is now accepted and ready to land. In D91311#2400998 , @dblaikie wrote: > A concrete/real-world example might be helpful, if you happen to have one on > hand. See what Rich

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith added a comment. In D91311#2400998 , @dblaikie wrote: > In D91311#2400926 , @ldionne wrote: > >> For instance, I can easily imagine a library that provides an API where some >> types shouldn't be named (for

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. In D91311#2400926 , @ldionne wrote: > In D91311#2400917 , @dblaikie wrote: > >> How would that work for users - they would get error messages from the >> compiler using type names that don

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne added subscribers: david_stone, mattcalabrese, mpark. ldionne added a comment. In D91311#2400917 , @dblaikie wrote: > How would that work for users - they would get error messages from the > compiler using type names that don't exist in the source

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. In D91311#2400775 , @ldionne wrote: > In D91311#2398526 , @rsmith wrote: > >> [...] > > Thanks for your detailed explanation. I did not understand the philosophy of > the attribute, and it

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne added a comment. In D91311#2398526 , @rsmith wrote: > [...] Thanks for your detailed explanation. I did not understand the philosophy of the attribute, and it's now clear to me that it shouldn't be tied to the typedef, indeed. > There's another

[PATCH] D91311: Add new 'preferred_name' attribute.

aaron.ballman added a comment. In D91311#2398526 , @rsmith wrote: > In D91311#2398144 , @ldionne wrote: > >> I think that the fact we need to re declare everything shows how the >> ergonomics would be better if we

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith marked an inline comment as done. rsmith added a comment. In D91311#2398144 , @ldionne wrote: > What the attribute achieves is great, however I must say I'm really with > Arthur's original comment regarding the ergonomics of it. I do agree, the er

[PATCH] D91311: Add new 'preferred_name' attribute.

ldionne added a comment. What the attribute achieves is great, however I must say I'm really with Arthur's original comment regarding the ergonomics of it. IMO, it makes a lot more sense to permit the typedef author to pick how their typedef is going to be "named" by the compiler. If they pick

[PATCH] D91311: Add new 'preferred_name' attribute.

Quuxplusone added inline comments. Comment at: libcxx/include/regex:2520 +_LIBCPP_PREFERRED_NAME(wregex) +basic_regex { aaron.ballman wrote: > rsmith wrote: > > Quuxplusone wrote: > > > Why does this attribute go on the class template? Shouldn't it be an

[PATCH] D91311: Add new 'preferred_name' attribute.

aaron.ballman added inline comments. Comment at: libcxx/include/regex:2520 +_LIBCPP_PREFERRED_NAME(wregex) +basic_regex { rsmith wrote: > Quuxplusone wrote: > > Why does this attribute go on the class template? Shouldn't it be an > > attribute on the ty

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith marked 2 inline comments as done. rsmith added inline comments. Comment at: libcxx/include/iosfwd:188 +#ifdef _LIBCPP_PREFERRED_NAME +template aaron.ballman wrote: > We always define `_LIBCPP_PREFERRED_NAME` so is this actually needed? Thanks, I was try

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith updated this revision to Diff 305222. rsmith added a comment. - Properly disable redundant redeclarations if preferred_name attribute is Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91311/new/ https://reviews.llvm.org/D91311 Files: clang

[PATCH] D91311: Add new 'preferred_name' attribute.

Quuxplusone added inline comments. Comment at: libcxx/include/regex:2520 +_LIBCPP_PREFERRED_NAME(wregex) +basic_regex { Why does this attribute go on the class template? Shouldn't it be an attribute on the typedef, so that you don't have to repeat yours

[PATCH] D91311: Add new 'preferred_name' attribute.

aaron.ballman accepted this revision. aaron.ballman added a comment. The attribute parts LGTM, but I did have a question about the libc++ parts. Comment at: clang/include/clang/Basic/Attr.td:2367 +def PreferredName : InheritableAttr { + let Spellings = [Clang<"preferred_name">

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith added inline comments. Comment at: clang/include/clang/Basic/Attr.td:2367 +def PreferredName : InheritableAttr { + let Spellings = [Clang<"preferred_name">]; + let Subjects = SubjectList<[ClassTmpl]>; aaron.ballman wrote: > This seems like one we should

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith updated this revision to Diff 304985. rsmith added a comment. - Remove debugging code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91311/new/ https://reviews.llvm.org/D91311 Files: clang/include/clang/Basic/Attr.td clang/include/clang

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith updated this revision to Diff 304984. rsmith marked 5 inline comments as done. rsmith added a comment. - Address review comments. - Extend libc++ changes to cover . - Bugfixes from further testing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D91311: Add new 'preferred_name' attribute.

aaron.ballman added a comment. I really like this attribute, thank you for working on this! Comment at: clang/include/clang/Basic/Attr.td:2367 +def PreferredName : InheritableAttr { + let Spellings = [Clang<"preferred_name">]; + let Subjects = SubjectList<[ClassTmpl]>; --

[PATCH] D91311: Add new 'preferred_name' attribute.

rsmith created this revision. rsmith added reviewers: aaron.ballman, EricWF. Herald added a subscriber: jdoerfert. Herald added projects: clang, libc++. Herald added a reviewer: libc++. rsmith requested review of this revision. This attribute permits a typedef to be associated with a class templat