[PATCH] D141280: [clang][wip] Build UsingType for elaborated type specifiers.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I'll bite, why do you say it's hacky?
Looks OK to me, by clang standards :-)



================
Comment at: clang/include/clang/Sema/DeclSpec.h:516
+  }
+  Decl *getRepAsOriginDecl() const {
+    assert(isDeclRep((TST)TypeSpecType) && "DeclSpec does not store a decl");
----------------
elsewhere we call this the **found** decl, seems worth sticking with the terminology unless there's a distinction I'm missing.

(I'd also add a comment here, even though it doesn't seem to be local style, sigh)


================
Comment at: clang/include/clang/Sema/Sema.h:3307
 
   Decl *ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                  SourceLocation KWLoc, CXXScopeSpec &SS, IdentifierInfo *Name,
----------------
hokein wrote:
> This is the key place where it returns the underlying decl.
> 
> Alternatively, we could return the UsingDecl as the result (there are only 4 places using it, might be possible to check/verify/adjust all caller place, I feel a bit scary about it, as the name indicates, caller might assume the return Decl is a `TagDecl`, e.g. the [case](https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplate.cpp#L10195))
Yeah, given the name I think returning the TagDecl makes sense.
(It looks like it should always return TagDecl, though it's hard to tell - if so then changing the static type to TagDecl* seems like it would help)

I think the extra out-param is fine, except given that there are few callsites, I'd prefer a mandatory `FoundUsingShadow*&` and explicitly ignoring it where appropriate - seems less cryptic and makes discarding sugar more obvious.


================
Comment at: clang/include/clang/Sema/Sema.h:3316
+                 SkipBodyInfo *SkipBody = nullptr,
+                 UsingShadowDecl** FoundUsingShadow = nullptr);
 
----------------
nit: needs clang-format for pointer-alignment here and elsewhere


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141280