[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

riccibruno added a comment. Upload patches with full context please. Also the diagnostic seems to be wrong ? And finally is there a bug report for this ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57111/new/ https://reviews.llvm.org/D57111 _

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

riccibruno added a comment. In D57111#1368230 , @rzhikharevich wrote: > In D57111#1368224 , @riccibruno > wrote: > > > Upload patches with full context please. > > Also the diagnostic seems to be wrong ? > > > I

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. In D57111#1368277 , @rzhikharevich wrote: > In D57111#1368256 , @riccibruno > wrote: > > > I guess yo

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1368292 , @steveire wrote: > In D57104#1368072 , @riccibruno > wrote: > > > In D57104#1368055 , @steveire > > wrote: > > > > > Splitti

[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

riccibruno abandoned this revision. riccibruno marked 2 inline comments as done. riccibruno added a comment. Closed in favor of D57104 and D57106 . Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57

[PATCH] D57114: Remove Expr sugar decorating the CXXUuidofExpr node.

riccibruno added inline comments. Comment at: lib/Sema/SemaTemplate.cpp:6311 if (isa(E)) { - Converted = TemplateArgument(ArgResult.get()); + Converted = TemplateArgument(ArgResult.get()->IgnoreImpCasts()); break; A remark wh

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5103 +using reference = AssociationTy; +using pointer = AssociationTy; +AssociationIteratorTy() = default; aaron.ballman wrote: > Carry

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno marked 2 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5095 - SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; } - SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; } --

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno marked 3 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5095 - SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; } - SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; } --

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno updated this revision to Diff 183314. riccibruno added a comment. Made `GenericSelectionExpr::AssociationIterator` a random access iterator. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57106/new/ https://reviews.llvm.org/D57106 Files: include/clan

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno updated this revision to Diff 183349. riccibruno marked 6 inline comments as done. riccibruno added a comment. Cleanup the patch by factoring out the NFC changes. @steveire This should be much clearer now. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno updated this revision to Diff 183352. riccibruno added a comment. Rebased on D57104 . No other changes. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57106/new/ https://reviews.llvm.org/D57106 Files: include/clang/AS

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1369864 , @steveire wrote: > Thanks, but I don't see the factored out changes in the repo. Are you going > to commit those first? > > Also, please factor out the introduction and use of `::Create`. It seems to > be u

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1369985 , @steveire wrote: > There's definitely a better possible ordering in two commits: > > 1. Introduce `::Create` and port to it > 2. Use trailing objects, taking advantage of the fact that `::Create` exists. > >

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1370025 , @aaron.ballman wrote: > In D57104#1369985 , @steveire wrote: > > > There's definitely a better possible ordering in two commits: > > > > 1. Introduce `::Create` and p

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. > I highly recommend this 9 minute video if this is new to you or you haven't > seen it before: https://youtu.be/qpdYRPL3SVE?t=103 I would like to add an additional meta-comment here, but please don't take this in a bad way. I am wondering about the usefulness and th

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1370275 , @steveire wrote: > In D57104#1370081 , @riccibruno > wrote: > > > > I highly recommend this 9 minute video if this is new to you or you > > > haven't seen it before:

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno marked 2 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5103 +using reference = AssociationTy; +using pointer = AssociationTy; +AssociationIteratorTy() = default; dblaikie wrote: > aaron.ball

[PATCH] D57238: [AST][NFC] Various cleanups to GenericSelectionExpr

riccibruno created this revision. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Various cleanups to GenericSelectionExpr, factored out of D57104 . In particular: 1. Move the friend declaration to the top. 2. Introduce a constant `Resu

[PATCH] D57104: [AST] Pack GenericSelectionExpr

riccibruno added a comment. In D57104#1371161 , @steveire wrote: > In D57104#1369751 , @riccibruno > wrote: > > > Cleanup the patch by factoring out the NFC changes. > > > > @steveire This should be much clearer no

[PATCH] D57238: [AST][NFC] Various cleanups to GenericSelectionExpr

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5095 + + Expr *getAssocExpr(unsigned i) { +return cast(SubExprs[AssocExprStartIndex + i]); aaron.ballman wrote: > If you wanted to fix up `i`

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno updated this revision to Diff 183552. riccibruno marked 10 inline comments as done. riccibruno added a comment. Moved back to an input iterator. Addressed Aaron's remaining comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57106/new/ https://revi

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5103 +using reference = AssociationTy; +using pointer = AssociationTy; +AssociationIteratorTy() = default; aaron.ballman wrote: > riccibruno wrote: > > dblaikie wrote: > > > aaron.

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno marked 3 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5084 + /// storage of Stmt * and TypeSourceInfo * in GenericSelectionExpr. + template class AssociationIteratorTy { +friend class GenericSelectionExpr; -

[PATCH] D57266: [AST] Update the comments of the various Expr::Ignore* + Related cleanups

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. The description of what the various `Expr::Ignore*` do has drifted from the actual implementation. Inspection reveals that `IgnoreParenImpCasts

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Now that the implementation of all of the `Expr::Ignore*` is in `Expr.cpp` we can try to remove some duplication. Do this by separating the logi

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno updated this revision to Diff 183658. riccibruno added a comment. Update the comment in the iterator as per Aaron's comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57106/new/ https://reviews.llvm.org/D57106 Files: include/clang/AST/Expr.h

[PATCH] D57266: [AST] Update the comments of the various Expr::Ignore* + Related cleanups

riccibruno updated this revision to Diff 183695. riccibruno added a comment. Removed superfluous braces added in some if statements. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57266/new/ https://reviews.llvm.org/D57266 Files: include/clang/AST/Expr.h incl

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno updated this revision to Diff 183696. riccibruno added a comment. Update the comment in `IgnoreImpCastsExtraSingleStep` and return early from `IgnoreImpCastsExtraSingleStep` and `IgnoreImplicitSingleStep` when `IgnoreImpCastsSingleStep` skipped something. Repository: rC Clang CHA

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5084 + /// storage of Stmt * and TypeSourceInfo * in GenericSelectionExpr. + template class AssociationIteratorTy { +friend class GenericSelectionExpr; -

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno updated this revision to Diff 183752. riccibruno added a comment. Use `iterator_facade_base`. It didn't work the first time because I forgot to bring in the name `operator++` from `iterator_facade_base` into `AssociationIteratorTy`. Since name lookup occurs before overload resolution

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno updated this revision to Diff 183755. riccibruno added a comment. Removed duplicated comment in `AssociationIteratorTy` Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57106/new/ https://reviews.llvm.org/D57106 Files: include/clang/AST/Expr.h includ

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno marked 3 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5122 +AssociationIteratorTy() = default; +AssociationTy operator*() const { + return AssociationTy(cast(*E), *TSI, aaron.ballman wrote

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno marked 5 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:5122 +AssociationIteratorTy() = default; +AssociationTy operator*() const { + return AssociationTy(cast(*E), *TSI, aaron.ballman wrote

[PATCH] D57266: [AST] Update the comments of the various Expr::Ignore* + Related cleanups

riccibruno marked 8 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:749-750 + /// reaching a fixed point. Skips: + /// * ImplicitCastExpr. + /// * FullExpr. Expr *IgnoreImpCasts() LLVM_READONLY; aaron.ballman

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno updated this revision to Diff 183865. riccibruno marked 4 inline comments as done. riccibruno added a comment. Removed the separator comments in `Expr.cpp` Mode `IgnoreExprNodes` work with an arbitrary number of functions. Repository: rC Clang CHANGES SINCE LAST ACTION https://re

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

riccibruno added inline comments. Comment at: lib/AST/Expr.cpp:2562 +static Expr *IgnoreImpCastsSingleStep(Expr *E) { + if (auto *ICE = dyn_cast_or_null(E)) +return ICE->getSubExpr(); aaron.ballman wrote: > Do we really need to accept null arguments? We didn

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

riccibruno marked an inline comment as done. riccibruno added a comment. In D57106#1375142 , @steveire wrote: > This was subsequently reverted. Is there a status update? Rebasing and > committing D56959 would unblock trav

[PATCH] D54161: [AST] Pack DeclRefExpr, UnaryOperator, MemberExpr and BinaryOperator

riccibruno created this revision. riccibruno added a reviewer: rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Move some data to the newly available space in the bit-fields of `Stmt`. This cuts the size of `DeclRefExpr`, `UnaryOperator`, `MemberExpr` and `BinaryO

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

riccibruno created this revision. riccibruno added a reviewer: rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt` and store the string data in a trailing array of `char`s after the trailing array of `SourceLo

[PATCH] D54172: [AST] Pack ArraySubscriptExpr and CallExpr

riccibruno created this revision. riccibruno added a reviewer: rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt` to store some data from `ArraySubscriptExpr` and `CallExpr`. This saves a pointer for each of

[PATCH] D54321: [AST] Store the expressions in ParenListExpr in a trailing array

riccibruno created this revision. riccibruno added a reviewer: rsmith. riccibruno added a project: clang. Herald added a reviewer: shafik. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt`, and store the expressions in `ParenListExpr` in a trailing a

[PATCH] D54324: [AST] Store the value of CharacterLiteral inline if possible

riccibruno created this revision. riccibruno added a reviewer: rsmith. riccibruno added a project: clang. Herald added a reviewer: shafik. Herald added a subscriber: cfe-commits. The vast majority of `CharacterLiteral`s have a value which fits into 8 bits (in the 2666 `CharacterLiteral`s in all of

[PATCH] D54325: [AST] Pack CXXBoolLiteralExpr, CXXNullPtrLiteralExpr and CXXThisExpr

riccibruno created this revision. riccibruno added a reviewer: rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt` to store some data from `CXXBoolLiteralExpr`, `CXXNullPtrLiteralExpr` and `CXXThisExpr`. This

[PATCH] D54326: [AST] Pack CXXThrowExpr, CXXDefaultArgExpr and CXXDefaultInitExpr

riccibruno created this revision. riccibruno added a reviewer: rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. riccibruno added a dependency: D54325: [AST] Pack CXXBoolLiteralExpr, CXXNullPtrLiteralExpr and CXXThisExpr. Use the newly available space in the bit-f

[PATCH] D54325: [AST] Pack CXXBoolLiteralExpr, CXXNullPtrLiteralExpr and CXXThisExpr

riccibruno updated this revision to Diff 173383. riccibruno added a comment. Fix some typos in the added comments in `Stmt.h` Repository: rC Clang https://reviews.llvm.org/D54325 Files: include/clang/AST/ExprCXX.h include/clang/AST/Stmt.h Index: include/clang/AST/Stmt.h

[PATCH] D54326: [AST] Pack CXXThrowExpr, CXXDefaultArgExpr and CXXDefaultInitExpr

riccibruno updated this revision to Diff 173652. Repository: rC Clang https://reviews.llvm.org/D54326 Files: include/clang/AST/ExprCXX.h include/clang/AST/Stmt.h lib/AST/ExprCXX.cpp lib/Serialization/ASTReaderStmt.cpp Index: lib/Serialization/ASTReaderStmt.cpp

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

riccibruno added a comment. @dblaikie Thanks for looking at this patch ! I have a set of patches shrinking the other statements/expressions. Can I add you to review some of these too ? Most of them consists of just moving some data. I added rsmith but I think that he is busy with the standard.

[PATCH] D54324: [AST] Store the value of CharacterLiteral in the bit-fields of Stmt if possible

riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:1407 public: - enum CharacterKind { -Ascii, -Wide, -UTF8, -UTF16, -UTF32 - }; + enum CharacterKind { Ascii, Wide, UTF8, UTF16, UTF32 }; shafik wrote: > Minor comment,

[PATCH] D53717: [AST] Only store the needed data in ForStmt.

riccibruno abandoned this revision. riccibruno added a comment. Herald added a reviewer: shafik. Let's close it for now. I agree that the complexity is probably not worth it. We can always resurrect it in the future. I will submit a patch doing the move into a new `StmtBase.h/.cpp` after I am do

[PATCH] D54524: [AST] Pack UnaryOperator

riccibruno created this revision. riccibruno added a reviewer: dblaikie. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt` to store some data from `UnaryOperator`. This saves 8 bytes per `UnaryOperator`. Repositor

[PATCH] D54161: [AST] Pack DeclRefExpr, UnaryOperator, MemberExpr and BinaryOperator

riccibruno abandoned this revision. riccibruno added a comment. Broken into pieces and sent individually for review. Repository: rC Clang https://reviews.llvm.org/D54161 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.or

[PATCH] D54525: [AST] Pack MemberExpr

riccibruno created this revision. riccibruno added a reviewer: dblaikie. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt` to store some data from `MemberExpr`. This saves one pointer per `MemberExpr`. Repository:

[PATCH] D54526: [AST] Pack BinaryOperator

riccibruno created this revision. riccibruno added a reviewer: dblaikie. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt`. This saves 8 bytes per `BinaryOperator`. Repository: rC Clang https://reviews.llvm.org

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

riccibruno updated this revision to Diff 174032. riccibruno added a comment. Factored out some NFSs which I will submit separately. Repository: rC Clang https://reviews.llvm.org/D54166 Files: include/clang/AST/Expr.h include/clang/AST/Stmt.h lib/AST/Expr.cpp lib/Serialization/ASTRead

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

riccibruno marked 2 inline comments as done. riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:1615 + } + + /// Build a string literal. Note that the trailing array of chars is aligned to 4 bytes since it is after the array of `SourceLocati

[PATCH] D54326: [AST] Pack CXXThrowExpr, CXXDefaultArgExpr and CXXDefaultInitExpr

riccibruno abandoned this revision. riccibruno added a comment. Broken down and sent for review individually. Repository: rC Clang https://reviews.llvm.org/D54326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D54325: [AST] Pack CXXBoolLiteralExpr, CXXNullPtrLiteralExpr and CXXThisExpr

riccibruno abandoned this revision. riccibruno added a comment. Broken down and sent for review individually. Repository: rC Clang https://reviews.llvm.org/D54325 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D54172: [AST] Pack ArraySubscriptExpr and CallExpr

riccibruno abandoned this revision. riccibruno added a comment. Sent for review individually. Repository: rC Clang https://reviews.llvm.org/D54172 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D54526: [AST] Pack BinaryOperator

riccibruno marked an inline comment as done. riccibruno added a comment. Added some inline comments. Comment at: include/clang/AST/Expr.h:3220 - SourceLocation getExprLoc() const LLVM_READONLY { return OpLoc; } - SourceLocation getOperatorLoc() const { return OpLoc; } - vo

[PATCH] D54525: [AST] Pack MemberExpr

riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:2802 - friend class ASTReader; - friend class ASTStmtWriter; }; Having this at the bottom is so painful for the reader, and people seems to put them at the top of the class usually. But yes

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

riccibruno marked 2 inline comments as done. riccibruno added a comment. In https://reviews.llvm.org/D54166#1298889, @rjmccall wrote: > IIRC, abbreviations just silently don't take effect if the record doesn't > conform; so things will appear to work, but the size on disk will be bigger. I loo

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

riccibruno added a comment. In https://reviews.llvm.org/D54166#1299230, @rjmccall wrote: > In https://reviews.llvm.org/D54166#1299149, @riccibruno wrote: > > > In https://reviews.llvm.org/D54166#1298889, @rjmccall wrote: > > > > > IIRC, abbreviations just silently don't take effect if the record

[PATCH] D54547: PTH-- Remove feature entirely-

riccibruno added a comment. I don't think I can comment more broadly on this, but there is some PTH-related code in `Basic/IdentifierTable`, in `IdentifierInfo::getNameStart` and `IdentifierInfo::getLength`. Maybe this should be removed too ? Repository: rC Clang https://reviews.llvm.org/D545

[PATCH] D54526: [AST] Pack BinaryOperator

riccibruno marked 6 inline comments as done. riccibruno added a comment. Marked the inline comments as done since I believe I answered each of them. If not I can fix it in a subsequent commit. Repository: rC Clang https://reviews.llvm.org/D54526

[PATCH] D54547: PTH-- Remove feature entirely-

riccibruno added a comment. In https://reviews.llvm.org/D54547#1299883, @erichkeane wrote: > Added to the release notes. Also, an email was sent out to cfe-dev. > > @riccibruno and I are separately looking into IdentifierInfo, because it > seems that there are some pretty massive optimization o

[PATCH] D54547: PTH-- Remove feature entirely-

riccibruno added a comment. > I thought clang-d service is using it to speed up indexing. I believe that clangd is using PCH and not PTH. (by the highly sophisticated method of grepping for pth and pch inside clangd/) https://reviews.llvm.org/D54547 __

[PATCH] D54675: [AST] Store the expressions in ParenListExpr in a trailing array

riccibruno created this revision. riccibruno added a reviewer: rjmccall. riccibruno added a project: clang. Herald added a reviewer: shafik. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt` and store the expressions in a trailing array. This saves

[PATCH] D54676: [AST] Pack CallExpr

riccibruno created this revision. riccibruno added a reviewer: rjmccall. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Use the newly available space in the bit-fields of `Stmt` to store some data from `CallExpr`. This saves 8 bytes per `CallExpr`. This is a straightfo

[PATCH] D54676: [AST] Pack CallExpr

riccibruno added a comment. In https://reviews.llvm.org/D54676#1302510, @rjmccall wrote: > I don't think we should be reducing the number of call arguments we can > support, sorry, even if 16K is a fairly absurd number that would probably > trip stack overflow protections if you actually execut

[PATCH] D54676: [AST] Pack CallExpr

riccibruno planned changes to this revision. riccibruno added a comment. In https://reviews.llvm.org/D54676#1303176, @rjmccall wrote: > You can have more arguments than parameters because of varargs. Even putting > that aside, no, I think we generally shouldn't go backwards on these limits. >

[PATCH] D54866: Cleanups in IdentifierInfo following the removal of PTH

riccibruno created this revision. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Conditional on D54547 : PTH-- Remove feature entirely- The `Entry` pointer in `IdentifierInfo` was only null for `IdentifierInfo`s created from a PTH. Now

[PATCH] D54866: Cleanups in IdentifierInfo following the removal of PTH

riccibruno updated this revision to Diff 175143. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54866/new/ https://reviews.llvm.org/D54866 Files: include/clang/Basic/IdentifierTable.h lib/Basic/IdentifierTable.cpp Index: lib/Basic/IdentifierTable.cpp

[PATCH] D54547: PTH-- Remove feature entirely-

riccibruno added a comment. I looked at how `ASTReader` create the `IdentifierInfo`s returned by `IdentifierInfo *ASTReader::get(StringRef Name)`, and I ended up in `ASTIdentifierLookupTrait::ReadData`, which calls among other things `IdentifierTable::getOwn`. The `IdentifierInfo` created by `ge

[PATCH] D54866: Cleanups in IdentifierInfo following the removal of PTH

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: include/clang/Basic/IdentifierTable.h:119 + IdentifierInfo() + : TokenID(tok::identifier), ObjCOrBuiltinID(0), HasMacro(false), erichkeane wrote: > Instead of putti

[PATCH] D54866: Cleanups in IdentifierInfo following the removal of PTH

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: include/clang/Basic/IdentifierTable.h:119 + IdentifierInfo() + : TokenID(tok::identifier), ObjCOrBuiltinID(0), HasMacro(false), erichkeane wrote: > riccibruno wrote

[PATCH] D54547: PTH-- Remove feature entirely-

riccibruno added inline comments. Comment at: include/clang/Lex/Preprocessor.h:396 + /// This is an alias for CurLexer. PreprocessorLexer *CurPPLexer = nullptr; erichkeane wrote: > riccibruno wrote: > > Would it make sense to remove this alias now that it >

[PATCH] D54900: [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType

riccibruno created this revision. riccibruno added reviewers: rsmith, aaron.ballman. riccibruno added a project: clang. Herald added subscribers: cfe-commits, dexonsmith, inglorion, mehdi_amini. `CallExpr::setNumArgs` is the only thing that prevents storing the arguments of a call expression in a

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

riccibruno created this revision. riccibruno added reviewers: rsmith, aaron.ballman. riccibruno added a project: clang. Herald added subscribers: cfe-commits, dexonsmith, mehdi_amini. `CallExpr::setNumArgs` is the only thing that prevents storing the arguments in a trailing array. There is only 3

[PATCH] D54900: [Sema] Avoid CallExpr::setNumArgs in Sema::BuildCallToObjectOfClassType

riccibruno updated this revision to Diff 175431. riccibruno marked 3 inline comments as done. riccibruno added a comment. fixed the formatting Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54900/new/ https://reviews.llvm.org/D54900 Files: lib/Sema/SemaOverload

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

riccibruno updated this revision to Diff 175434. riccibruno marked 9 inline comments as done. riccibruno added a comment. style fixes Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54902/new/ https://reviews.llvm.org/D54902 Files: include/clang/AST/Expr.h inc

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

riccibruno added a comment. added inline comments Comment at: include/clang/AST/ExprCXX.h:168 public: - CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args, -QualType t, ExprValueKind VK, SourceLocation RP) - : CallExpr(C, CXXMemberCallExprClass,

[PATCH] D52879: Derive builtin return type from its definition

riccibruno added a comment. I see plenty of `TheCall->setType` left in `Sema::CheckBuiltinFunctionCall` (`Builtin::BI__builtin_classify_type`, `Builtin::BI__builtin_constant_p`, `Builtin::BI__builtin_dump_struct` and so on...). Is there a reason for not removing them ? Repository: rC Clang C

[PATCH] D52879: Derive builtin return type from its definition

riccibruno added a comment. In D52879#1309856 , @mantognini wrote: > In D52879#1309734 , @riccibruno > wrote: > > > I see plenty of `TheCall->setType` left in `Sema::CheckBuiltinFunctionCall` > > (`Builtin::BI__bu

[PATCH] D52879: Derive builtin return type from its definition

riccibruno added a comment. And moreover I believe this change is subtly incorrect for the following reason: The type that is passed into the constructor of the call expression is the type of the call expression, and not the return type. The difference between the return type and the type of the

[PATCH] D52879: Derive builtin return type from its definition

riccibruno added a comment. In D52879#1311177 , @riccibruno wrote: > And moreover I believe this change is subtly incorrect for the following > reason: > The type that is passed into the constructor of the call expression is the > type > of the call ex

[PATCH] D52879: Derive builtin return type from its definition

riccibruno added a comment. In D52879#1314694 , @mantognini wrote: > In D52879#1311177 , @riccibruno > wrote: > > > And moreover I believe this change is subtly incorrect for the following > > reason: > > The typ

[PATCH] D52879: Derive builtin return type from its definition

riccibruno added a comment. In D52879#1314855 , @mantognini wrote: > Thank you for the detailed review. I'll work on a patch and add you as > reviewer once done (prob. on Monday though). That's fine. Thanks for your contribution ! Repository: rC Cla

[PATCH] D55136: [OpenCL][Sema] Improve BuildResolvedCallExpr handling of builtins

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. Much better. LGTM with a small format nit. Comment at: lib/Sema/SemaExpr.cpp:5556 // Extract the return type from the (builtin) function pointer type. -auto

[PATCH] D55221: [AST] Make ArrayTypeTraitExpr non-polymorphic.

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. `ArrayTypeTraitExpr` is the only expression class which is polymorphic. As far as I can tell this is completely pointless. Repository: rC Cla

[PATCH] D55222: [AST] Assert that no statement/expression class is polymorphic

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Add a `static_assert` checking that no statement/expression class is polymorphic. People should use LLVM style RTTI instead. Repository: rC

[PATCH] D55225: [AST] Assert that no type class is polymorphic

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Add a `static_assert` checking that no type class is polymorphic. People should use LLVM style RTTI instead. Repository: rC Clang https://re

[PATCH] D55225: [AST] Assert that no type class is polymorphic

riccibruno marked 2 inline comments as done. riccibruno added inline comments. Comment at: lib/AST/Type.cpp:299 + static_assert(!std::is_polymorphic::value, \ +#CLASS "Type should not be polymorphic!"); +#include "clang/AST/TypeNodes.def" aaron.b

[PATCH] D55222: [AST] Assert that no statement/expression class is polymorphic

riccibruno updated this revision to Diff 176435. riccibruno added a comment. Forgot a space in the error message Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55222/new/ https://reviews.llvm.org/D55222 Files: lib/AST/Stmt.cpp Index: lib/AST/Stmt.cpp

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

riccibruno added a comment. In D54737#1317128 , @JonasToth wrote: > I had to revert and recommitted in rCTE348169 > . `std::unordered_map Something, ...>` does not work, as `std::hash` is not specialized for it. > Thi

[PATCH] D54866: Cleanups in IdentifierInfo following the removal of PTH

riccibruno added a comment. Can this go in now that D54547 has been committed ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54866/new/ https://reviews.llvm.org/D54866 ___ cfe-comm

[PATCH] D55334: [AST] Pass the ASTContext to one of the ctor of DeclRefExpr

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Pass the `ASTContext` to one of the constructor of `DeclRefExpr` since in most cases the appropriate `ASTContext` is readily available. `Decl::g

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

riccibruno added a comment. In D55395#1324699 , @aaron.ballman wrote: > This is a novel approach that's not used anywhere else in the AST dumper and > there are several ways we could handle this, including: > > - What's proposed (adding a new node to the

[PATCH] D55534: [AST] Store "UsesADL" information in CallExpr.

riccibruno added inline comments. Comment at: include/clang/AST/Expr.h:2425 CallExpr(const ASTContext &C, StmtClass SC, unsigned NumPreArgs, - unsigned NumArgs, EmptyShell Empty); + unsigned NumArgs, bool UsesADL, EmptyShell Empty); There

[PATCH] D55534: [AST] Store "UsesADL" information in CallExpr.

riccibruno added inline comments. Comment at: lib/AST/Expr.cpp:1267 : Expr(SC, Empty), NumArgs(NumArgs) { + CallExprBits.UsesADL = false; CallExprBits.NumPreArgs = NumPreArgs; It do not really matter but there is not point initializing this bit here. C

<    1   2   3   4   5   6   7   >