[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

riccibruno created this revision. riccibruno added reviewers: erichkeane, rjmccall. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Since Decls are already aligned explicitly to 8 bytes, stash the 3 bits representing an LVComputationKind into the lower 3 bits of the Name

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

riccibruno added inline comments. Comment at: include/clang/AST/DeclarationName.h:243 + /// C++ literal operator, or C++ using directive. uintptr_t Ptr = 0; erichkeane wrote: > There is an llvm type for storing something in the lower bits of a pointer.

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

riccibruno added a comment. In https://reviews.llvm.org/D52268#1239538, @erichkeane wrote: > Does this still work with 32 bit hosts? Does PointerIntPair have 3 bits in > that case? Is the alignof static_assert valid in that case? I think it does since Decl is manually over-aligned to 8 bytes

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

riccibruno updated this revision to Diff 166264. riccibruno marked 3 inline comments as done. riccibruno added a comment. Removed the superfluous static_assert. Repository: rC Clang https://reviews.llvm.org/D52268 Files: lib/AST/Linkage.h Index: lib/AST/Linkage.h

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

riccibruno updated this revision to Diff 166324. riccibruno marked 9 inline comments as done. riccibruno added a comment. Address rjmccall comments: 1. Renamed `CXXSpecialName` to `CXXSpecialNameExtra` 2. Introduced a constant `IdentifierInfoAlignment` for `alignas(IdentifierInfoAlignment)` 3. U

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

riccibruno added a comment. Addressed some inline comments. Comment at: include/clang/AST/DeclarationName.h:46 -/// DeclarationName - The name of a declaration. In the common case, -/// this just stores an IdentifierInfo pointer to a normal -/// name. However, it also provide

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

riccibruno added a comment. In https://reviews.llvm.org/D52268#1241033, @rjmccall wrote: > `LinkageComputer` isn't actually persisted anywhere, right? And there's > maybe one computer active at once? So this compression is theoretically > saving one pointer of stack space but forcing a bunch

[PATCH] D52738: [AST] Pack the bit-fields of FunctionProtoType into Type.

riccibruno created this revision. riccibruno added a reviewer: rjmccall. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Move the bit-fields of FunctionProtoType into FunctionTypeBitfields. This cuts the size of FunctionProtoType by a pointer. Additionally use llvm::Tra

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

riccibruno abandoned this revision. riccibruno added a comment. Superseded by https://reviews.llvm.org/D52738. Repository: rC Clang https://reviews.llvm.org/D50631 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-

[PATCH] D57645: [C++2a] Fix PR40576: Turn destroying delete off prior to C++2a. Add -fdestroying-delete

riccibruno added inline comments. Comment at: lib/AST/DeclCXX.cpp:2070 + if (isDestroyingOperatorDelete()) { +if (!getASTContext().getLangOpts().DestroyingDelete) + return false; Just use the AST context you have above ? Repository: rC Clang CHANGE

[PATCH] D57592: Replace uses of %T with %t in from previous frontend test differential

riccibruno added a comment. > The NetBSD buildbot breaks in these tests now: Pretty much every bot is red because of this patch (http://lab.llvm.org:8011/one_line_per_build) so yes, please fix. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57592/new/ https://rev

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

riccibruno added inline comments. Comment at: lib/AST/Expr.cpp:2562 +return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null(E)) It is something that is actually possible to audit. I did look at where each of the skipped node are created and it *

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

riccibruno updated this revision to Diff 184944. riccibruno marked 6 inline comments as done. riccibruno added a comment. Rebased and addressed Aaron's comments, except for the null check issue on which I am still undecided. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.ll

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

riccibruno updated this revision to Diff 184945. riccibruno marked 4 inline comments as done. riccibruno added a comment. Addressed Aaron's comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57266/new/ https://reviews.llvm.org/D57266 Files: include/clang/

[PATCH] D57649: [ASTDump] Add a flag indicating whether a CXXThisExpr is implicit

riccibruno created this revision. riccibruno added a reviewer: steveire. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. There is current no way to distinguish implicit from explicit `CXXThisExpr` in the AST dump output. Repository: rC Clang https://reviews.llvm.or

[PATCH] D57649: [ASTDump] Add a flag indicating whether a CXXThisExpr is implicit

riccibruno added a comment. In D57649#1382453 , @steveire wrote: > I have no objection to this, but I wonder whether all state accessible from > all nodes should be part of the AST dump. Where do you think the line is? Is > there anything else missing cu

[PATCH] D57649: [ASTDump] Add a flag indicating whether a CXXThisExpr is implicit

riccibruno added a comment. Thanks for the review ! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57649/new/ https://reviews.llvm.org/D57649 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.

[PATCH] D57659: [Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

riccibruno created this revision. riccibruno added reviewers: aaron.ballman, rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Do the following NFCs, before doing anything more substantial: 1. Add some comments to hopefully reduce the amount of code staring for th

[PATCH] D57660: [Sema] SequenceChecker: Handle references and members

riccibruno created this revision. riccibruno added reviewers: aaron.ballman, rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Teach the unsequenced operation checker about references and members. To do this introduce a class `MemoryLocation` which will approximat

[PATCH] D57661: [Sema] SequenceChecker: Add tests for references and members.

riccibruno created this revision. riccibruno added reviewers: aaron.ballman, rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Tests for D57660 . Repository: rC Clang https://reviews.llvm.org/D57661 Files: test/SemaCXX/warn-

[PATCH] D57649: [ASTDump] Add a flag indicating whether a CXXThisExpr is implicit

riccibruno added a comment. In D57649#1382477 , @aaron.ballman wrote: > In D57649#1382470 , @riccibruno > wrote: > > > Though it is a subjective distinction and in the end if someone need a flag > > it is going t

[PATCH] D57665: [clang-tidy] Handle unions with existing default-member-init

riccibruno added a comment. Just to make sure I understand the issue correctly (and so please correct me if I am wrong): The union member `e` leads to an `IndirectFieldDecl` in the record for `UnionExisting`. Then `CXXCtorInitializer::getMember` returns null since the initialized member is not

[PATCH] D57661: [Sema] SequenceChecker: Add tests for references and members.

riccibruno added a comment. I realized I need to add tests for (at least) anonymous unions/structs and local classes, but I am sure I missing lots of interesting cases. Feel free to suggest anything you might think of. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D57661: [Sema] SequenceChecker: Add tests for references and members.

riccibruno updated this revision to Diff 185047. riccibruno added a comment. Added tests for nested/anonymous unions and local structs. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57661/new/ https://reviews.llvm.org/D57661 Files: test/SemaCXX/warn-unsequence

[PATCH] D57660: [Sema] SequenceChecker: Handle references and members

riccibruno updated this revision to Diff 185048. riccibruno added a comment. Added tests for nested/anonymous unions and local structs. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57660/new/ https://reviews.llvm.org/D57660 Files: include/clang/Basic/Diagnost

[PATCH] D57659: [Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: lib/Sema/SemaChecking.cpp:11748 +/// The expressions corresponding to each usage kind. +Expr *UsageExprs[UK_Count]{}; + aaron.ballman wrote: > You can drop the `{}`

[PATCH] D57747: [Sema] SequenceChecker: Fix handling of operator ||, && and ?:

riccibruno created this revision. riccibruno added reviewers: aaron.ballman, rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. The current handling of the operators `||`, `&&` and `?:` has a number of false positive and false negative. The issues for operator `||`

[PATCH] D57086: Ignore trailing NullStmts in StmtExprs for GCC compatibility

riccibruno added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1260 + // this compound expression contains nothing but NullStmts, then we return + // the index of the last one. If the compound statement is empty, return + // None. Additionally I

[PATCH] D57747: [Sema] SequenceChecker: Fix handling of operator ||, && and ?:

riccibruno added a comment. Looking at git blame, this change means that we will warn on `(b && x++) + (!b && x++)`. However we will also warn on an expression like `(x > some_constant && y++) + (x < some_constant && y++)`. This seems to be the job of a static analyser which is able to do some

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

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: lib/AST/Expr.cpp:2562 +return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null(E)) aaron.ballman wrote: > riccibruno wrote: > > It is something that is actu

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

riccibruno updated this revision to Diff 185539. riccibruno added a comment. Go back to using `dyn_cast`s. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57267/new/ https://reviews.llvm.org/D57267 Files: include/clang/AST/Expr.h lib/AST/Expr.cpp Index: lib/A

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. D54902 removed `CallExpr::setNumArgs` in preparation of tail-allocating the arguments of `CallExpr`. It did th

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

riccibruno added subscribers: rnk, void. riccibruno added a comment. @void @rnk Perhaps you can comment on this: currently `Expr::IgnoreImpCasts` skips `FullExpr`s, but `Expr::IgnoreParenImpCasts` only skips (via `IgnoreParens`) `ConstantExpr`s. Is there any reason for this inconsistency ? I wo

[PATCH] D40720: No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17

riccibruno added a comment. Herald added a project: clang. It seems to be causing https://bugs.llvm.org/show_bug.cgi?id=40658. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40720/new/ https://reviews.llvm.org/D40720

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

riccibruno added a comment. > I don't think there was an explicit reason beyond "I didn't need to do it at > the time". So probably just an oversight on my part. I don't know the code > nearly as well as @rnk, so I could be wrong, but I think the existing tests > should tell you if something we

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

riccibruno added a comment. > FWIW, I was rather disappointed in a recent review to learn that > IgnoreParens() means "ignore parens... and a whole bunch of other stuff like > generic selection expressions". +1 Indeed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

riccibruno updated this revision to Diff 186449. riccibruno added a comment. Rewrote the patch to make it local to `BuildResolvedCallExpr`. It is still a hack though. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57948/new/ https://reviews.llvm.org/D57948 Files

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

riccibruno added a subscriber: hans. riccibruno added a comment. (Following up on a discussion on IRC) I have looked at what would be needed to revert the set of patches which introduced this change, but this results in a >1k lines diff which do not apply cleanly. I think it might be safer to ju

[PATCH] D57660: [Sema] SequenceChecker: Handle references and members

riccibruno updated this revision to Diff 187035. riccibruno added a comment. Herald added a subscriber: jdoerfert. Rebased. Does this implementation make sense ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57660/new/ https://reviews.llvm.org/D57660 Files: in

[PATCH] D58297: [Sema] SequenceChecker: C++17 sequencing rules for built-in operators <<, >>, .*, ->*, =, op=

riccibruno created this revision. riccibruno added reviewers: rsmith, rjmccall, aaron.ballman. riccibruno added a project: clang. Herald added subscribers: cfe-commits, jdoerfert. Implement the C++17 sequencing rules for the built-in operators `<<`, `>>`, `.*`, `->*`, `=` and `op=`. Repository:

[PATCH] D57747: [Sema] SequenceChecker: Fix handling of operator ||, && and ?:

riccibruno updated this revision to Diff 187057. riccibruno marked an inline comment as done. riccibruno added a comment. Herald added a subscriber: jdoerfert. Rebased Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57747/new/ https://reviews.llvm.org/D57747 Files

[PATCH] D57948: [Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

riccibruno added a comment. Done, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57948/new/ https://reviews.llvm.org/D57948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bi

[PATCH] D58297: [Sema] SequenceChecker: C++17 sequencing rules for built-in operators <<, >>, .*, ->*, =, op=

riccibruno updated this revision to Diff 187089. riccibruno added a comment. 1. Only call `note{Pre,Post}{Use, Mod}` if we have a valid memory location. 2. Fixed 2 tests in `CXX/drs` which I initially missed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58297/new

[PATCH] D57659: [Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

riccibruno updated this revision to Diff 187090. riccibruno marked an inline comment as done. riccibruno added a comment. Rebased and fixed a comment. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57659/new/ https://reviews.llvm.org/D57659 Files: lib/Sema/Sema

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added a comment. In D57914#1401367 , @pgousseau wrote: > Updated patch to not use APInt for representing the mask as it feels like a > hammer solution. Ah! I was going to say exactly this. CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added inline comments. Comment at: include/clang/Basic/Sanitizers.h:43 + // Low bits of mask value. + uint64_t maskLo; + // High bits of mask value. Why not use a fixed size array of `uint64_t`s, and then compute the index without any branch with `

[PATCH] D57660: [Sema] SequenceChecker: Handle references and members

riccibruno planned changes to this revision. riccibruno added a comment. This needs more work. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57660/new/ https://reviews.llvm.org/D57660 ___ cfe-commits mailing list cfe

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno marked an inline comment as done. riccibruno added a comment. Looks mostly fine to me. I have added a few inline comments but they are all little nits. Comment at: include/clang/Basic/Sanitizers.h:39 +struct SanitizerMask { +private: + // Number of array elements.

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno accepted this revision. riccibruno marked an inline comment as done. riccibruno added a comment. This revision is now accepted and ready to land. LGTM with an additional comment. Comment at: include/clang/Basic/Sanitizers.h:81 + + explicit operator bool() { +for

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno requested changes to this revision. riccibruno added a comment. This revision now requires changes to proceed. Wait no, can you really define the `SanitizerMask`s in the header ? Isn't that an odr violation ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57914/new/ https://r

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added a comment. I think what you would really want to do is mark the masks as `inline constexpr`, but sadly inline variables are C++17 only. I have seen some ways to emulate inline variables but I am not sure whether it is worth doing so in this case. CHANGES SINCE LAST ACTION h

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added a comment. In D57914#1405665 , @pgousseau wrote: > In D57914#1405615 , @riccibruno > wrote: > > > I think what you would really want to do is mark the masks as `inline > > constexpr`, but sadly in

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added a comment. To define the masks as `constexpr` variables you will have to make at least `bitPosToMask`, `operator|`, `operator&` and `operator~`. Unfortunately `constexpr` functions in c++11 are rather restricted. It seems to me however that you could do the following: 1. Wait

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added a comment. More explicitly something like: in `Sanitizer.h`: template struct SanitizerMasks { static const SanitizerMask SomeMask; /* and so on for each mask*/ }; template const SanitizerMask SanitizerMasks::SomeMask = the definition; And then you can write

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

riccibruno added a comment. If this bit is relevant to function parameters, why is `getIsArgumentModified` in `VarDecl` and not in `ParamVarDecl` ? What you can do is move the relevant methods to `ParamVarDecl`, and stash the bit in `ParmVarDeclBitfields`. Comment at: include

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

riccibruno added a comment. Oh and I think that you will also have to update the serialization code/de-serialization code in `ASTReaderDecl.cpp` / `ASTWriterDecl.cpp`. You might also have to update `TreeTransform` but I am less familiar with this. CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added inline comments. Comment at: include/clang/Basic/Sanitizers.h:148 +// workaround from n4424 to avoid odr issues. +// FIXME: Can be replaced by constexpr once c++14 can be used in llvm. +template struct SanitizerMasks { Not replaced. `constexpr`

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

riccibruno added a comment. I am wondering about the semantics of this bit. I don't think that you can know for sure within clang whether a variable has been modified. The best you can do is know for sure that some variable has been modified, but I don't think you can prove that it has not been

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added inline comments. Comment at: include/clang/Basic/Sanitizers.h:173 + SanitizerMask::bitPosToMask(SO_##ID##Group); \ + static const SanitizerMask &ID##Group = SanitizerMasks<>::ID##Group; #include "clang/Basic/Sanitizers.def" ---

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

riccibruno added inline comments. Comment at: lib/Sema/SemaExpr.cpp:11301 +EmitArgumentsValueModification(E); + SourceLocation OrigLoc = Loc; Comments: 1. Shouldn't you mark the variable to be modified only if `CheckForModifiableLvalue` returns true ? 2

[PATCH] D57660: [Sema] SequenceChecker: Handle references, members and structured bindings.

riccibruno updated this revision to Diff 188043. riccibruno retitled this revision from "[Sema] SequenceChecker: Handle references and members" to "[Sema] SequenceChecker: Handle references, members and structured bindings.". riccibruno edited the summary of this revision. Repository: rC Clang

[PATCH] D57747: [Sema] SequenceChecker: Fix handling of operator ||, && and ?:

riccibruno updated this revision to Diff 188045. riccibruno added a comment. Rebased on D57660 . No need to look at it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57747/new/ https://reviews.llvm.org/D57747 Files: lib/Sema/Se

[PATCH] D58297: [Sema] SequenceChecker: C++17 sequencing rules for built-in operators <<, >>, .*, ->*, =, op=

riccibruno updated this revision to Diff 188046. riccibruno added a comment. Rebased on D57660 . No need to look at it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58297/new/ https://reviews.llvm.org/D58297 Files: lib/Sema/Se

[PATCH] D57659: [Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

riccibruno updated this revision to Diff 188048. riccibruno added a comment. Rebased. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57659/new/ https://reviews.llvm.org/D57659 Files: lib/Sema/SemaChecking.cpp Index: lib/Sema/SemaChecking.cpp ==

[PATCH] D58579: [Sema] SequenceChecker: C++17 sequencing rule for call expression.

riccibruno created this revision. riccibruno added reviewers: rsmith, aaron.ballman, Rakete. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. In C++17 the postfix-expression of a call expression is sequenced before each expression in the expression-list and any defau

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

riccibruno added a comment. > I'm not quite sure what this differential is about, but i feel like > mentioning ExprMutationAnalyzer lib in clang-tidy / clang-tools-extra. Alternatively perhaps you could re-use `getMemoryLocation()` from D57660 . It would handle

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno added a comment. Hmm. These are not the only static variables which are used for statistics (eg: in `DeclBase.cpp`). Would it make sense instead to keep all of the statistics in the AST context (without making them atomic) ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno added a comment. In D58612#1408836 , @ilya-biryukov wrote: > In D58612#1408831 , @riccibruno > wrote: > > > Hmm. These are not the only static variables which are used for statistics > > (eg: in `DeclB

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno added a comment. In D58612#1408843 , @ilya-biryukov wrote: > In D58612#1408839 , @riccibruno > wrote: > > > I don't know how hard it would be to do this, but I would like to argue > > that this should

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

riccibruno added a comment. It looks better now as far as I can see. I like the idea of aliasing `SanitizerKind` to `SanitizerMasks<>;`. Comment at: include/clang/Basic/Sanitizers.h:133 // bit positions. enum SanitizerOrdinal : uint64_t { #define SANITIZER(NAME, ID) SO_##ID

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno added a comment. Okay, but what about the other similar uses of static members which have the same problem ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58612/new/ https://reviews.llvm.org/D58612 ___

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

riccibruno added inline comments. Comment at: lib/Sema/SemaExpr.cpp:11292 +} + +/// Argument's value might be modified, so update the info. Hmm, I don't think that this will work. Suppose that you have an expression like `(a, b) += c` You want to mark `b` as mo

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno added a comment. In D58612#1408991 , @alexfh wrote: > In D58612#1408942 , @riccibruno > wrote: > > > Okay, but what about the other similar uses of static members which have > > the same problem ? > > >

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. Sounds good. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58612/new/ https://reviews.llvm.org/D58612 _

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno added a comment. (With a commit message which actually reflect the change of course) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58612/new/ https://reviews.llvm.org/D58612 ___ cfe-commits

[PATCH] D58612: Use std::atomic<> for static counters.

riccibruno added a comment. In D58612#1409215 , @alexfh wrote: > > In D58612#1409024 , @riccibruno > > wrote: > > > >> ... > >> For example in `DeclBase.cpp` > >> > >> #define DECL(DERIVED, BASE) static int n##

[PATCH] D63276: [AST] Add FunctionDecl::getParametersSourceRange()

riccibruno added inline comments. Comment at: clang/include/clang/AST/Type.h:4048 +return *getTrailingObjects(); + } + And what if there is no ellipsis ? Shouldn't you do something like `return isVariadic() ? *getTrailingObjects() : SourceLocation();` inst

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

riccibruno added a comment. It seems that these two options are not exactly the same right ? The `ContainsError` bit is useful to quickly answer "Does this expression contains an invalid sub-expression" without doing the search, while adding an `ErrorExpr` node is useful to note that //this// s

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

riccibruno added a comment. In D65591#1625183 , @aaron.ballman wrote: > In D65591#1625154 , @riccibruno > wrote: > > > It seems that these two options are not exactly the same right ? The > > `ContainsError` bit

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

riccibruno added a comment. In D65591#1625228 , @ilya-biryukov wrote: > In D65591#1625183 , @aaron.ballman > wrote: > > > In D65591#1625154 , @riccibruno > > wrote: > > >

[PATCH] D61027: Fix crash on switch conditions of non-integer types in templates

riccibruno added a comment. I think that you have to run `check-clang-tools`. I too was surprised that it was not included in `check-clang`. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61027/new/ https://reviews.llvm.org/D61027 __

[PATCH] D57660: [Sema] SequenceChecker: Handle references, members and structured bindings.

riccibruno added a comment. In D57660#1637220 , @xbolva00 wrote: > Added some reviewers > > Patch improves suboptimal diagnostic, which misses bugs like: > https://bugs.llvm.org/show_bug.cgi?id=43052 Thanks for taking a look ! Repository: rC Clang

[PATCH] D57659: [Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

riccibruno updated this revision to Diff 216152. riccibruno added a comment. Rebased. Thanks for the review @xbolva00 ! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57659/new/ https://reviews.llvm.org/D57659 Files: clang/lib/Sema/SemaChecking.cpp Index: clan

[PATCH] D57660: [Sema] SequenceChecker: Handle references, members and structured bindings.

riccibruno updated this revision to Diff 216153. riccibruno added a comment. Rebased Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57660/new/ https://reviews.llvm.org/D57660 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/include/clang/Sema/Sem

[PATCH] D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator

riccibruno added a comment. Ping :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60956/new/ https://reviews.llvm.org/D60956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mail

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

riccibruno added inline comments. Comment at: include/clang/AST/Type.h:3692 FunctionType::ExceptionType, Expr *, FunctionDecl *, - FunctionType::ExtParameterInfo> { friend class ASTContext; // ASTContext creates these. You can use `Function

[PATCH] D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added subscribers: cfe-commits, arphaman. Herald added a reviewer: shafik. `Decl::getASTContext` and `DeclContext::getParentASTContext` are not that cheap since they must walk ba

[PATCH] D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1

riccibruno added a comment. Just to expend on the instrumentation results: The measurement was done with all of Boost. I would take the estimation of the time wasted with a grain of salt since this is just `num_iterations * 10ns` which is obviously a very rough estimation. However on my machine

[PATCH] D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1

riccibruno updated this revision to Diff 178259. riccibruno marked 3 inline comments as done. riccibruno added a comment. Addressed inline comments Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55658/new/ https://reviews.llvm.org/D55658 Files: include/clang/AS

[PATCH] D55737: [Utils] Attempt to fix clang.natvis following "[AST] Various optimizations in DeclarationName(Table)"

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. `[AST] Various optimizations in DeclarationName(Table)` changed how `DeclarationName`s are represented. Therefore `clang.natvis` needs to be upd

[PATCH] D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1

riccibruno added a comment. In D55658#1332240 , @vsk wrote: > Thanks for working on this :). It’d be interesting to see some pre/post patch > compile time numbers on CTMark. > > Naive/strawman alternative here, but: what’s the impact on peak RSS and > co

[PATCH] D53708: [ASTImporter] Add importer specific lookup

riccibruno added a comment. This is a perhaps a naive comment, but why is `localUncachedLookup` used instead of `noload_lookup` ? There is a fixme dating from 2013 stating that `noload_lookup` should be used instead. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5

[PATCH] D53708: [ASTImporter] Add importer specific lookup

riccibruno added a comment. In D53708#1332868 , @martong wrote: > > This is a perhaps a naive comment, but why is localUncachedLookup used > > instead of noload_lookup ? There is a fixme dating from 2013 stating > > that noload_lookup should be used inst

[PATCH] D53708: [ASTImporter] Add importer specific lookup

riccibruno added a comment. > Exactly. And this seems to be an important feature in ASTImporter, because > this makes it possible that we can chain into the redecl chain a newly > imported AST node to existing and structurally equivalent > nodes. (The > existing nodes are found by the lookup an

[PATCH] D55771: [AST] Store the callee and argument expressions of CallExpr in a trailing array.

riccibruno created this revision. riccibruno added a reviewer: rjmccall. riccibruno added a project: clang. Herald added subscribers: cfe-commits, javed.absar. Herald added a reviewer: shafik. Since `CallExpr::setNumArgs` has been removed, it is now possible to store the callee expression and the

[PATCH] D54676: [AST] Pack CallExpr

riccibruno abandoned this revision. riccibruno added a comment. Abandoned in favor of D55771 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54676/new/ https://reviews.llvm.org/D54676

[PATCH] D55790: [AST] Add accessSpecs() iterator range for CXXRecordDecl

riccibruno added a comment. Do you really have to add an iterator for this particular thing ? Why not just use `specific_decl_iterator` in you clang-tidy checker ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55790/new/ https://reviews.llvm.org/D55790 __

[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

riccibruno added a comment. In D55793#1333691 , @m4tx wrote: > Don't use `CXXRecordDecl::accessSpecs()`, use unique comments in tests. Thanks! `CXXRecordDecl` is already huge and so adding iterators for a single checker is in my opinion not a good idea

[PATCH] D55771: [AST] Store the callee and argument expressions of CallExpr in a trailing array.

riccibruno updated this revision to Diff 178641. riccibruno marked an inline comment as done. riccibruno added a comment. Make `OffsetToTrailingObjects` byte sized and aligned to a byte multiple. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55771/new/ https://re

[PATCH] D55737: [Utils] Attempt to fix clang.natvis following "[AST] Various optimizations in DeclarationName(Table)"

riccibruno abandoned this revision. riccibruno added a comment. In D55737#1335245 , @aaron.ballman wrote: > There was a bit more work to be done here, but this got me started. I commit > extra fixes on top of your work in r349547 since I was able to test

<    1   2   3   4   5   6   7   >