cor3ntin marked 6 inline comments as done.
cor3ntin added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1411
let Documentation = [DeprecatedDocs];
+ let ParseArgumentsAsUnevaluated = 1;
}
----------------
aaron.ballman wrote:
> What is the plan for non-standard attributes? Are you planning to handle
> those in a follow-up, or should we be investigating those right now?
I don't feel I'm qualified to answer that. Ideally, attributes that expect
string literals that are not evaluated should follow suite.
================
Comment at: clang/include/clang/Parse/Parser.h:1857-1859
bool FailImmediatelyOnInvalidExpr = false,
- bool EarlyTypoCorrection = false);
+ bool EarlyTypoCorrection = false,
+ bool AllowEvaluatedString = true);
----------------
aaron.ballman wrote:
> Two default `bool` params is a bad thing but three default `bool` params
> seems like we should fix the interface at this point. WDYT?
>
> Also, it's not clear what the new parameter will do, the function could use
> comments unless fixing the interface makes it sufficiently clear.
I'm still not sure that's the best solution. `AllowEvaluatedString` would only
ever be false for attributes, I consider duplicating the function, except it
does quite a bit for variadics, which apparently attribute support
Maybe would could have
```
ParseAttributeArgumentList
ParseExpressionList
ParseExpressionListImpl?
```
?
================
Comment at: clang/lib/AST/Expr.cpp:1165
+ unsigned CharByteWidth = mapCharByteWidth(Ctx.getTargetInfo(), Kind);
+ unsigned ByteLength = Str.size();
+ assert((ByteLength % CharByteWidth == 0) &&
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > shafik wrote:
> > > Isn't this the same as `Length`?
> > It is -- I think we can get rid of `ByteLength`, but it's possible that
> > this exists because of the optimization comment below. I don't insist, but
> > it would be nice to know if we can replace the switch with `Length /=
> > CharByteWidth` these days.
> Only when CharByteWidth == 1
I think we should.
================
Comment at: clang/lib/Lex/LiteralSupport.cpp:98
+ case 't':
+ case 'r':
+ return true;
----------------
aaron.ballman wrote:
> We're still missing support for some escape characters from:
> http://eel.is/c++draft/lex#nt:simple-escape-sequence-char
>
> Just to verify, UCNs have already been handled by the time we get here, so we
> don't need to care about those, correct?
> Just to verify, UCNs have already been handled by the time we get here, so we
> don't need to care about those, correct?
They are dealt with elsewhere yes (and supported)
================
Comment at: clang/lib/Lex/LiteralSupport.cpp:2080-2082
+ if (!isUnevaluated() && Features.PascalStrings &&
+ ThisTokBuf + 1 != ThisTokEnd && ThisTokBuf[0] == '\\' &&
+ ThisTokBuf[1] == 'p') {
----------------
aaron.ballman wrote:
> Is there test coverage that we diagnose this properly?
What sort of test would you like to see?
================
Comment at: clang/lib/Parse/ParseExpr.cpp:3501-3503
+ } else if (!AllowEvaluatedString && tok::isStringLiteral(Tok.getKind())) {
+ Expr = ParseUnevaluatedStringLiteralExpression();
+ } else {
----------------
aaron.ballman wrote:
> I'm surprised we need special logic in `ParseExpressionList()` for handling
> unevaluated string literals; I would have expected that to be needed when
> parsing a string literal. Nothing changed in the grammar for
> http://eel.is/c++draft/expr.post.general#nt:expression-list (or
> initializer-list), so these changes seem wrong. Can you explain the changes a
> bit more?
We use `ParseExpressionList` when parsing attribute arguments, and some
attributes have unevaluate string as argument - I agree with you that I'd
rather find a better solution for attributes, but I came up empty. There is no
further reason for this change, and you are right it does not match the grammar.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:878-879
- if (!isIntOrBool(AL.getArgAsExpr(0))) {
+ Expr *First = AL.getArgAsExpr(0);
+ if (isa<StringLiteral>(First) || !isIntOrBool(First)) {
S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
----------------
aaron.ballman wrote:
> Test coverage for these changes?
There is one somewhere, I don;t remember where, The reason we need to do that
is that Unevaluated StringLiterals don''t have types
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16470
StringLiteral *Lit = cast<StringLiteral>(LangStr);
- if (!Lit->isOrdinary()) {
- Diag(LangStr->getExprLoc(), diag::err_language_linkage_spec_not_ascii)
- << LangStr->getSourceRange();
- return nullptr;
- }
+ assert(Lit->isUnevaluated() && "Unexpected string literal kind");
----------------
aaron.ballman wrote:
> Test coverage for changes?
There are some in dcl.link/p2.cpp
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105759/new/
https://reviews.llvm.org/D105759
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
