sammccall added a comment.
OK, so I learned a bit about initializeLocal/trivial TypeSourceInfo. Mostly
things I didn't want to know :-)
TL;DR I think the current version of the patch is right.
The crash is caused by:
1. `CheckTemplateArgument` calls `SubstType` without passing a `TypeLoc`, which
synthesizes a trivial `TypeSourceInfo` to instantiate
2. the trivial `TypeSourceInfo` never has a `ConceptReference`
3. instantiating an `AutoTypeLoc` where the type is constrained but there's no
`ConceptReference` is not supported
We can choose to address any of these.
Fixing #1 is tempting: the SubstType overload with no TypeLoc is deprecated and
calling it ~always loses information. Cleaning up this call is an improvement,
but there are ~15 calls to this overload, and probably more places that use
trivial TypeSourceInfo.
So this is nice to have, but I think leaves us with an AST that will still
crash in other cases. Consumers will not expect an AutoTypeLoc where the type
is constrained but the loc isn't, and it's hard to know how to handle this case.
Fixing #2 is what this patch currently does. It also seems to be the pattern
used by other types (their initializeLocal methods conditionally initialize
structure based on the underlying type). The only possible downside is because
we store the ConceptReference subobject by pointer and support replacement but
not mutation, if the caller calls initializeLocal and then fills in with real
data we'll waste the original ConceptReference object. Still, I don't think
this is how non-trivial TSIs are generally created, and I think this is the way
to go.
Fixing #3 is the previous version of the patch, again it leaves us with a
bug-prone AST and some extra complexity.
================
Comment at: clang/include/clang/AST/ASTConcept.h:183
+ if (auto QualifierLoc = getNestedNameSpecifierLoc())
+ return QualifierLoc.getBeginLoc();
+ return getConceptNameInfo().getBeginLoc();
----------------
if the qualifier is null the template KW must also be null
so this is correct, you may or may not want a comment for that! up to you
================
Comment at: clang/include/clang/AST/ASTConcept.h:188
+ SourceLocation getEndLoc() const LLVM_READONLY {
+ return getTemplateArgsAsWritten()->getRAngleLoc().isValid()
+ ? getTemplateArgsAsWritten()->getRAngleLoc()
----------------
getTemplateArgsAsWritten() is nullable, right?
================
Comment at: clang/include/clang/AST/ASTConcept.h:215
+
+ void print(llvm::raw_ostream &OS, PrintingPolicy Policy) const;
};
----------------
nit: printingpolicy by const ref, I think
(we're not totally consistent, but I think ref is more common)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155858/new/
https://reviews.llvm.org/D155858
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
