[PATCH] D55413: [ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

riccibruno added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:3427 -if (!SubobjType->isIntegerType() || !RHS.isInt()) { +if (!SubobjType->isIntegerType() || !RHS.isInt() && !RHS.isFloat()) { // We don't support compound assignment on integer-cast-t

[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; I believe that msan can cope with bit level operations just fine. What I am af

[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; riccibruno wrote: > I believe that msan can cope with bit level operations jus

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

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. I think this looks good now. Thanks ! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57914/new/ https://reviews.llvm.org/D57914 _

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

riccibruno requested changes to this revision. riccibruno added inline comments. This revision now requires changes to proceed. Comment at: include/clang/Basic/Sanitizers.h:29 +class hash_code; +} What ? You are forward-declaring `hash_code` here and using it a

[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:29 +class hash_code; +} pgousseau wrote: > riccibruno wrote: > > What ? You are forward-declaring `hash_code` here and using it as a value a > > few lines later. Just include `llvm/

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

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. Looks good, hopefully this time it will stick. thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57914/new/ https://reviews.llvm.org/D57914

[PATCH] D58827: [Sema][NFCI] Don't heap-allocate the various CorrectionCandidateCallback unless we are going to do some typo correction

riccibruno created this revision. riccibruno added reviewers: aaron.ballman, rnk, erik.pilkington. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. The various `CorrectionCandidateCallback`s are currently heap-allocated unconditionally. This was needed because of delayed

[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

riccibruno added a comment. Ugh, could you please avoid doing lots a tiny changes every 5 minutes ? This causes spam on cfe-commits /: Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58818/new/ https://reviews.llvm.org/D58818 _

[PATCH] D58321: [WIP] Support for relative vtables

riccibruno added a comment. I think that you need to update the serialization code in `Serialization/ASTReaderDecl.cpp` and `Serialization/ASTWriterDecl.cpp` to account for the new flag. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58321/new/ ht

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

riccibruno added a comment. In D57914#1416215 , @jyknight wrote: > The intricate initialization-order workarounds apparently don't work in all > build modes, so I've updated this code to have constexpr functions and > initializations in rL355278

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added a comment. IIRC, last time I looked only 4 statement/expression classes currently have some abbreviation defined. It would probably be useful to have someone go systematically through the list of classes and fix this. Repository: rC Clang CHANGES SINCE LAST ACTION https:/

[PATCH] D59196: [NFC][clang][PCH] ASTStmtReader::VisitStmt(): fixup faulty assert.

riccibruno added a comment. I am not an expert in the serialization code (just did some modifications), but this seems reasonable to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59196/new/ https://reviews.llvm.org/D59196 _

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added a comment. In D59214#1429400 , @lebedev.ri wrote: > In D59214#1429384 , @riccibruno > wrote: > > > IIRC, last time I looked only 4 statement/expression classes currently have > > some abbreviation

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

riccibruno added a comment. Added some context in the description of the patch. Ping ! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57660/new/ https://reviews.llvm.org/D57660 ___ cfe-commits mailing list cfe-commits

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

riccibruno updated this revision to Diff 190671. riccibruno added a comment. Don't visit the pre-argument expression(s). A pre-argument expression is only present for `CUDAKernelCallExpr`, and it is just a `CallExpr` for the configuration call, which I don't think we need to visit. This makes th

[PATCH] D59196: [NFC][clang][PCH] ASTStmtReader::VisitStmt(): fixup faulty assert.

riccibruno added a comment. In D59196#1431473 , @lebedev.ri wrote: > In D59196#1429393 , @riccibruno > wrote: > > > I am not an expert in the serialization code (just did some modifications), > > but this seems re

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: include/clang/AST/Stmt.h:100 unsigned sClass : 8; +unsigned IsOMPStructuredBlock : 1; }; What about a comment explaining the purpose of this bit ? Comment at: include/clang/AST/StmtOpenM

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:274 + /// Prerequisite: Executable Directive must not be Standalone directive. + const Stmt *getStructuredBlock() const; }; Uh, and what about the non-const version of `getStructuredB

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: lib/AST/StmtOpenMP.cpp:40 + if (auto *LD = dyn_cast(this)) +return LD->getBody(); + return getInnermostCapturedStmt()->getCapturedStmt(); lebedev.ri wrote: > @riccibruno > `getBody()` exists in `const`-only vari

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

riccibruno added inline comments. Comment at: lib/AST/StmtOpenMP.cpp:40 + if (auto *LD = dyn_cast(this)) +return LD->getBody(); + return getInnermostCapturedStmt()->getCapturedStmt(); lebedev.ri wrote: > riccibruno wrote: > > lebedev.ri wrote: > > > @riccib

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

riccibruno added a comment. This is causing https://bugs.llvm.org/show_bug.cgi?id=41171. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59076/new/ https://reviews.llvm.org/D59076 ___ cfe-commits mailing list cfe-commi

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

riccibruno added a comment. Friendly ping. One thing I am wondering about is whether `MemoryLocation` and `getMemoryLocation` is duplicating something that is already present somewhere else. It feels like something similar should already exist but I can't find anything (but that is not saying m

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. Additionally I think that you need to: - add tests for templates - handle the ternary operator Also, as per the coding guidelines, you need to fix: - spelling of variables, eg `hint` -> `Hint`. - run `clang-format` on your patch. Comment at: clang

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added inline comments. Comment at: clang/lib/Sema/SemaStmtAttr.cpp:74 + ControlScope->getFlags() & Scope::FnTryCatchScope) + S.Diag(A.getLoc(), diag::err_must_appear_after_branch) << A.getName(); + } I think that you need to test for each

[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

riccibruno added a comment. In D59076#1438479 , @riccibruno wrote: > This is causing https://bugs.llvm.org/show_bug.cgi?id=41171. @modocache Could you take a look please ? Nested scopes in a catch statement are completely broken because of this patch.

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. Hmm, it seems that an attribute is not allowed by the grammar in the `expression` or `assignment-expression` of a `conditional-expression`. Was that intended when `[[likely]]` was added ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://revi

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. In D59467#1439485 , @aaron.ballman wrote: > In D59467#1439473 , @riccibruno > wrote: > > > Hmm, it seems that an attribute is not allowed by the grammar in the > > `expression` or `ass

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. To expand on the above, if you need to add a few bits to a statement/expression you can use the bit-field classes in `Stmt` (for example: `IfStmtBitfields`). You will also need to update the serialization code in `Serialization/ASTWriterStmt.cpp` and `Serialization/A

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. Before any further review, could you please run `clang-format` on your patch (but not necessarily on the tests and not on the `*.td` files), wrap lines to 80 cols, and in general use complete sentences in comments (that is with proper punctuation and capitalization)

[PATCH] D59752: Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block"

riccibruno added inline comments. Comment at: lib/Parse/ParseStmt.cpp:2293 // FIXME: Possible draft standard bug: attribute-specifier should be allowed? StmtResult Block(ParseCompoundStatement()); if (Block.isInvalid()) Just to make sure I understood the

[PATCH] D59752: Un-revert "[coroutines][PR40978] Emit error for co_yield within catch block"

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. Great! Since this already received one round of reviews I guess this looks okay. Comment at: test/SemaCXX/exceptions.cpp:25 + int ref = k; +} int j = i;

[PATCH] D59776: [Sema] Don't check for array bounds when the types in the base expression are dependent.

riccibruno created this revision. riccibruno added reviewers: efriedma, aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Bail-out of `CheckArrayAccess` when the types of the base expression before and after eventual casts are dependent. We will get another

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

riccibruno added a comment. Herald added a subscriber: dexonsmith. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57660/new/ https://reviews.llvm.org/D57660 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D60029: Add const children() accessors to Stmts

riccibruno added a comment. +1. I wanted to do this but never bothered. Did you go over all the statements on `Stmt.h` systematically ? You could also do the same thing for all of the statements/expressions in `StmtCXX.h`, `Expr.h`, `ExprCXX.h`, and so on. Apart from this did you run `clang-for

[PATCH] D60029: Add const children() accessors to Stmts

riccibruno added a comment. In D60029#1448944 , @nicolas wrote: > In D60029#1448938 , @riccibruno > wrote: > > > Did you go over all the statements on `Stmt.h` systematically ? > > > Yes. > > > You could also do th

[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a

riccibruno added a subscriber: NoQ. riccibruno added a comment. It seems that the tests are not present in this diff ? Also, again, could you please: 1. Use `clang-format`, and 2. Make sure that the comments are full sentences with appropriate punctuation, and 3. Follow the style guide regardin

[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a

riccibruno added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:705 +} + void CodeGenFunction::EmitWhileStmt(const WhileStmt &S, Tyker wrote: > riccibruno wrote: > > I believe that the lowering is incorrect. I applied your patch and here > > ({F85718

[PATCH] D60123: [AST] Forbid copy/move of Stmt.

riccibruno created this revision. riccibruno added a reviewer: aaron.ballman. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Statements and expressions are not supposed to be moved, and trying to do so is only going to result in tears. Someone tripped on this a few day

[PATCH] D60123: [AST] Forbid copy/move of Stmt.

riccibruno added a comment. In D60123#1451303 , @lebedev.ri wrote: > Probably `Decl` too? Yep. I though it was already done but after checking it seems I remembered incorrectly. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D60123: [AST] Forbid copy/move of statements/declarations/types.

riccibruno updated this revision to Diff 193267. riccibruno retitled this revision from "[AST] Forbid copy/move of Stmt." to "[AST] Forbid copy/move of statements/declarations/types.". riccibruno edited the summary of this revision. riccibruno added a reviewer: lebedev.ri. Repository: rC Clang

[PATCH] D60123: [AST] Forbid copy/move of statements/declarations/types.

riccibruno added a comment. In D60123#1451307 , @riccibruno wrote: > In D60123#1451303 , @lebedev.ri > wrote: > > > Probably `Decl` too? > > > Yep. I though it was already done but after checking it seems I remembe

[PATCH] D60123: [AST] Forbid copy/move of statements/types.

riccibruno updated this revision to Diff 193271. riccibruno retitled this revision from "[AST] Forbid copy/move of statements/declarations/types." to "[AST] Forbid copy/move of statements/types.". Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60123/new/ https://r

[PATCH] D60379: Make precompiled headers reproducible

riccibruno added inline comments. Comment at: lib/Serialization/ASTWriter.cpp:4283 + // Sort to allow reproducible .pch files - https://bugs.debian.org/877359 + std::map> sortedOpenCLTypeExtMap; for (const auto &I : SemaRef.OpenCLTypeExtMap) { lebedev.ri wro

[PATCH] D60379: Make precompiled headers reproducible

riccibruno added a comment. After looking at the bug report (https://bugs.debian.org/877359), I would like to point out that there is currently *absolutely* no stability guarantee for the serialization format (ie, you need the exact same revision). In regard of this I am wondering how sane it i

[PATCH] D60379: Make precompiled headers reproducible

riccibruno added a comment. In D60379#1458799 , @thakis wrote: > In D60379#1458668 , @riccibruno > wrote: > > > After looking at the bug report (https://bugs.debian.org/877359), I would > > like to point out that

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno created this revision. riccibruno added reviewers: rjmccall, Quuxplusone. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. The goal here is to exercise each rule in [basic.lookup.argdep] at least once. These new tests expose what I believe are 2 issues: 1. CW

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

riccibruno created this revision. riccibruno added reviewers: rjmccall, Quuxplusone. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. riccibruno added a parent revision: D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup. CWG 1691 changed th

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

riccibruno added a comment. Friendly ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57660/new/ https://reviews.llvm.org/D57660 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-b

[PATCH] D59221: [asan] Add gcc 8's driver option -fsanitize=pointer-compare and -fsanitize=pointer-substract.

riccibruno resigned from this revision. riccibruno added a comment. I am not the best person to review this, sorry! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59221/new/ https://reviews.llvm.org/D59221 ___ cfe-commits mailing list cfe-co

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno updated this revision to Diff 194763. riccibruno added a comment. Herald added a subscriber: eraman. Also test for inline namespaces. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60570/new/ https://reviews.llvm.org/D60570 Files: test/CXX/basic/bas

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

riccibruno updated this revision to Diff 194765. riccibruno added a comment. Removed the call to `isTransparentContext()` in the loop in `CollectEnclosingNamespace`. It is not actually needed since we only care about finding the innermost enclosing namespace. Repository: rC Clang CHANGES SI

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

riccibruno added a comment. In D60573#1463569 , @rjmccall wrote: > Hmm. Does this never impact code that's just using a locally-defined type > within its scope? I guess if ADL is involved, unqualified lookup must have > reached the scope of the innermo

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

riccibruno added a comment. In D60573#1463611 , @rsmith wrote: > > I have applied the fix to all language versions, even though I think that > > the DR only applies to C++14 > > DRs don't have a specific version that they are intended to apply to; that's

[PATCH] D60029: Add const children() accessors to all AST nodes.

riccibruno added a comment. Sorry for the delay. Thanks a lot for the patch! Some of these casts are rather questionable. However this has nothing to do with your patch since you are just adding the const-qualified version of `children()`. Therefore I believe this patch looks good. Do you want

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:304 +static_assert(f(g3) == 4, "");// FIXME: Also well-formed from the union rule. +

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

riccibruno added a comment. Friendly ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58579/new/ https://reviews.llvm.org/D58579 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-b

[PATCH] D60561: [clang] fixing diagnostics of constexpr callstack

riccibruno added a comment. In D60561#1464740 , @Tyker wrote: > @rsmith i don't think collecting theses values is expansive compared to > evaluating the expression. > but i agree that we can disable the collection of these values when no > diagnostics c

[PATCH] D60561: [clang] fixing diagnostics of constexpr callstack

riccibruno added a comment. In D60561#1465373 , @Tyker wrote: > the impact was much higher than i expected, around 1% slower in average on > 50 compilation of SemaDecl with -fsyntax-only. Good to know, thanks for doing the measurement ! CHANGES SINCE

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno marked 11 inline comments as done. riccibruno added a comment. In D60570#1465478 , @Quuxplusone wrote: > As you're making tests for ADL corner cases, you might also consider testing > the interactions between ADL and defaulted function paramete

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno updated this revision to Diff 195060. riccibruno marked 3 inline comments as done. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60570/new/ https://reviews.llvm.org/D60570 Files: test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespace

[PATCH] D60665: [Sema] ADL: Template arguments in a template-id naming a set of overloaded functions (part of CWG 997)

riccibruno created this revision. riccibruno added reviewers: Quuxplusone, rsmith, rjmccall. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. riccibruno added a parent revision: D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691). C

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

riccibruno added a comment. In D60573#1463777 , @rjmccall wrote: > In D60573#1463641 , @riccibruno > wrote: > > > In D60573#1463569 , @rjmccall > > wrote: > > > > > Hmm. D

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno updated this revision to Diff 195076. riccibruno added a comment. Also test that typedef-names and using-declarations are not considered. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60570/new/ https://reviews.llvm.org/D60570 Files: test/CXX/basic

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno marked 2 inline comments as done. riccibruno added a comment. (I have numbered your examples to make the discussion easier, and moved to a non-inline comment to have more space) foo(ft_t); // OK: ADL considers N::foo (nobody but GCC implements this) #1 foo(vt_t); // Error: ADL

[PATCH] D60665: [Sema] ADL: Template arguments in a template-id naming a set of overloaded functions (part of CWG 997)

riccibruno added a comment. For information we are discussing in D60570 whether it is actually a good idea to implement this rule. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60665/new/ https://reviews.llvm.org/D60665 _

[PATCH] D60732: [analyzer] NFC: Use -verify=... in MoveChecker tests.

riccibruno added a comment. This is also very useful to test that a given warning is only emitted in c++xx. Funnily when grepping for `verify=` in `test/` most matches are from tests exercising this functionality. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno added subscribers: aaron.ballman, rsmith. riccibruno added a comment. @rsmith @aaron.ballman Do you have any opinion on whether the ADL rules from CWG 997 should be implemented ? The issue here as I understand it is that these rules expand ADL, but no one apart from GCC implement them

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

riccibruno added a comment. Mmm. I hope I am not missing something obvious, but how is this actually fixing the issue ? I don't see why the order between the `Type *` and between the `Decl *` should be deterministic (I think they will be when they are part of the same slab in the allocator, but

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

riccibruno added a comment. Maybe, but is there an equivalent to `getTypeID` for declarations ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60778/new/ https://reviews.llvm.org/D60778 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

riccibruno created this revision. riccibruno added reviewers: Anastasia, lebedev.ri. riccibruno added a project: clang. Herald added subscribers: cfe-commits, mgrang, yaxunl. Sort the elements of `Sema::OpenCLTypeExtMap` and `Sema::OpenCLDeclExtMap` by `TypeID`s and `DeclID`s to guarantee a stabl

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

riccibruno added a comment. What about something like D60835 ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60778/new/ https://reviews.llvm.org/D60778 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

riccibruno added a comment. By the way, I am wondering about how much this is tested. I did look quickly in `test/PCH` and it appears that there are only 3 (short) tests : `ocl_types.cl`, `opencl-extensions.cl` and `no-validate-pch`. Repository: rC Clang CHANGES SINCE LAST ACTION https://

[PATCH] D60835: [Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap

riccibruno added a comment. In D60835#1471498 , @Anastasia wrote: > In D60835#1470805 , @riccibruno > wrote: > > > By the way, I am wondering about how much this is tested. I did look > > quickly in `test/PCH` and

[PATCH] D60910: [WIP] Dumping the AST to JSON

riccibruno added a subscriber: lebedev.ri. riccibruno added a comment. A few comments/questions: 1. How stable is the format going to be, and how much state is going to be exposed ? 2. I am wondering if it would be possible to separate the logic about how to visit a given AST node, from the log

[PATCH] D60934: [clang] adding explicit(bool) from c++2a

riccibruno added a comment. This patch certainly took a few hours to write. Can you spend 5 minutes on making the summary readable (spelling, capitalization, formatting, well-formed sentences, ...) ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60934/new/ http

[PATCH] D60910: [WIP] Dumping the AST to JSON

riccibruno added a comment. In D60910#1473484 , @aaron.ballman wrote: > In D60910#1473441 , @riccibruno > wrote: > > > A few comments/questions: > > > > 1. How stable is the format going to be, and how much state

[PATCH] D29707: Fix improper microsoft-pure-definition warning on template class

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. This looks reasonable to me (although I think that the test should be in `SemaCXX/` and not in `Parser/`, but the test for the non-template case is already in `Parser/` so ok). CHANG

[PATCH] D50360: [Concepts] Requires Expressions

riccibruno added inline comments. Comment at: include/clang/AST/ExprCXX.h:4671 + bool IsSatisfied; + SourceLocation RequiresKWLoc; + RequiresExprBodyDecl *Body; You can stash these in the bit-field classes of `Stmt` to save some space. Comme

[PATCH] D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle)

riccibruno created this revision. riccibruno added reviewers: aaron.ballman, rnk, rsmith. riccibruno added a project: clang. Herald added a subscriber: cfe-commits. Currently the lookup for a declaration conflicting with an enumerator is pretty broken, because of the use of `LookupResult::getAsSi

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno added a comment. @Quuxplusone Do you have other objections apart from the template-id issue ? If not, since D60573 depends on this patch, I would like to commit this with a comment explaining the issue instead of the FIXME. Repository: rC Clang

[PATCH] D60523: [clang] Don't segfault on incorrect using directive (PR41400)

riccibruno added a reviewer: riccibruno. riccibruno added a comment. I will take a look at this tomorrow, I know that it is annoying to get no feedback! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60523/new/ https://reviews.llvm.org/D60523 _

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

riccibruno accepted this revision. riccibruno added a comment. This revision is now accepted and ready to land. In D60570#1473873 , @Quuxplusone wrote: > > @Quuxplusone Do you have other objections apart from the template-id issue > > ? If not, since D605

[PATCH] D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle)

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: lib/Sema/SemaDecl.cpp:16607 + // Check for other kinds of shadowing not already handled. + if (PrevDecl && isa(PrevDecl->getUnderlyingDecl()) && + !TheEnumDecl->isScoped()) -

[PATCH] D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle)

riccibruno updated this revision to Diff 196264. riccibruno added a comment. - Added a test (see `N_shadow)` for the behavior of `Wshadow`. This test showed that I forgot to change `CheckShadow(New, PrevDecl, R);` to `CheckShadow(New, PrevDecl->getUnderlyingDecl(), R);` to match change in the co

[PATCH] D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle)

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: lib/Sema/SemaDecl.cpp:16607 + // Check for other kinds of shadowing not already handled. + if (PrevDecl && isa(PrevDecl->getUnderlyingDecl()) && + !TheEnumDecl->isScoped()) -

[PATCH] D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle)

riccibruno added a comment. > Added a test which exposes a new problem that this patch incidentally solves > (see `N_conflicting_namespace_alias`). Because of the using directive `using > namespace M;`, the namespace alias `i` for the namespace `Q` is found in the > redeclaration lookup. Before

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

riccibruno commandeered this revision. riccibruno edited reviewers, added: Anastasia; removed: riccibruno. riccibruno added a comment. Closing this since the issue was fixed in r358674. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60778/new/ https://reviews.llvm.org/D60778 _

[PATCH] D60956: [Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle)

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: test/CXX/dcl.dcl/dcl.enum/p11-enumerator_scope.cpp:107 enum { typedef_type };// expected-error {{redefinition of 'typedef_type'}} }; Note also that t

[PATCH] D60523: [clang] Don't segfault on incorrect using directive (PR41400)

riccibruno resigned from this revision. riccibruno added a comment. In D60523#1473867 , @riccibruno wrote: > I will take a look at this tomorrow, I know that it is annoying to get no > feedback! Sorry, I don't think I can judge whether this is the corre

[PATCH] D61225: [COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI

riccibruno added a comment. Since this is target-specific, is it right to put this here ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61225/new/ https://reviews.llvm.org/D61225 ___ cfe-commits mailing list cfe-comm

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

riccibruno updated this revision to Diff 197225. riccibruno retitled this revision from "[Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle)" to "[Sema] Fix the lookup for a declaration conflicting with an enumerator". riccibruno edited

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

riccibruno marked 3 inline comments as done. riccibruno added inline comments. Comment at: test/CXX/dcl.dcl/dcl.enum/p11-enumerator_scope.cpp:107 enum { typedef_type };// expected-error {{redefinition of 'typedef_type'}} }; riccibruno wrote

[PATCH] D61288: [Diagnostics] Implemented support for -Wswitch-default

riccibruno added a comment. Is it intentional to warn even if all the cases are covered ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61288/new/ https://reviews.llvm.org/D61288 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D60665: [Sema] ADL: Template arguments in a template-id naming a set of overloaded functions (part of CWG 997)

riccibruno added a comment. @rsmith Do you have an opinion on whether this ADL rule should be implemented ? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60665/new/ https://reviews.llvm.org/D60665 ___ cfe-commits mai

[PATCH] D61288: [Diagnostics] Implemented support for -Wswitch-default

riccibruno added inline comments. Comment at: include/clang/Basic/DiagnosticIDs.h:39 DIAG_SIZE_CROSSTU = 100, - DIAG_SIZE_SEMA = 3500, + DIAG_SIZE_SEMA = 3600, DIAG_SIZE_ANALYSIS = 100, xbolva00 wrote: > @rsm

[PATCH] D61389: Bump DIAG_SIZE_SEMA up to 4000

riccibruno added a comment. Already done in r359702 :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61389/new/ https://reviews.llvm.org/D61389 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D60910: [WIP] Dumping the AST to JSON

riccibruno marked an inline comment as done. riccibruno added inline comments. Comment at: include/clang/AST/JSONNodeDumper.h:55 + } + + /// Add a child of the current node with an optional label. aaron.ballman wrote: > riccibruno wrote: > > Perhaps you should

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

riccibruno updated this revision to Diff 198629. riccibruno marked 4 inline comments as done. riccibruno added a comment. Address Aaron's comments. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60956/new/ https://reviews.llvm.org/D60956 Files: lib/Sema/SemaDec

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

riccibruno created this revision. riccibruno added reviewers: erichkeane, rjmccall. riccibruno added a project: clang. Herald added subscribers: cfe-commits, dexonsmith, mehdi_amini. Introduce the following optimizations in DeclarationName(Table) 1. Store common kinds inline in DeclarationName in

<    1   2   3   4   5   6   7   >