[PATCH] D114025: [clang][NFC] Inclusive terms: replace some uses of sanity in clang

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:9176
       // Don't check the implicit member of the anonymous union type.
-      // This is technically non-conformant, but sanity demands it.
+      // This is technically non-conformant, but validation tests demand it.
       return false;
----------------
ZarkoCA wrote:
> Quuxplusone wrote:
> > Quuxplusone wrote:
> > > Quuxplusone wrote:
> > > > This comment seems incorrectly translated.
> > > This comment //still// seems incorrectly translated.
> > > Things we do "for sanity's sake" aren't necessarily //required//, technically; but we're doing them anyway, for sanity.
> > "Don't check ... but check it anyway"?
> Right, that didn't make sense :). I noticed that there were warnings for this case in SemaDecl.cpp AFAIU so edited the comment to state that. Should be better now? 
Well, this version of the comment now gives the //impression// that it must be clear to //someone//. ;) I don't understand its implications, but I also don't know the code.

Specifically, I don't know what "This" refers to (grammatically) — "Anonymous union types aren't conforming, but we support them anyway, and this is the right thing to do with them"? "Our behavior in this case isn't conforming, but it wouldn't make sense to do the conforming thing [wat]"? or something else?

More fundamentally to //my// confusion (but not to that hypothetical other someone), I don't know what "the implicit member of the anonymous union type" actually means (in terms of the arcane details of C++), i.e., I personally don't know when this codepath is hit or what its effect is when it does get hit.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:12260
   // even if hidden by ordinary names, *except* in a dependent context
-  // where it's important for the sanity of two-phase lookup.
+  // where it's important for the validation of two-phase lookup.
   if (!IsInstantiation)
----------------
Quuxplusone wrote:
> This comment seems incorrectly translated.
Perhaps just `// where they may be used by two-phase lookup.` (But this is now just a nit.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114025/new/

https://reviews.llvm.org/D114025