[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

tahonermann added a comment. Apologies once again for the delayed response. I reviewed some today and will resume reviewing on Monday. In addition to the inline suggestions: `clang/docs/ReleaseNotes.rst` will need to be updated to reflect that the core language changes for P1467R9 have been im

[PATCH] D156565: Diagnose use of VLAs in C++ by default

tahonermann added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149 +def ext_vla_cxx_static_assert : ExtWarn< + "variable length arrays in C++ are a Clang extension; did you mean to use " + "'static_assert'?">, InGroup; aaron

[PATCH] D156565: Diagnose use of VLAs in C++ by default

tahonermann added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149 +def ext_vla_cxx_static_assert : ExtWarn< + "variable length arrays in C++ are a Clang extension; did you mean to use " + "'static_assert'?">, InGroup; I fin

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

tahonermann added a comment. @codemzs, I'm sorry for the very long delay following up on this review. I've been struggling to keep up, but expect to be able to devote some time to this next week. I'm committed to helping to ensure we land this before Phabricator stops accepting new diffs (propo

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG256a0b298c68: [clang] Correct source locations for instantiations of function templates. (authored by tahonermann). Repository: rG LLVM Github Mon

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

tahonermann updated this revision to Diff 556970. tahonermann added a comment. Addressed review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64087/new/ https://reviews.llvm.org/D64087 Files: clang/docs/ReleaseNotes.rst clang/lib/Sem

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

tahonermann added a comment. @cor3ntin, any concerns or suggestions per my recent updates? I'll plan to land this in the next couple of days otherwise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64087/new/ https://reviews.llvm.org/D64087 _

[PATCH] D159474: [NFC][CLANG] Fix static analyzer bugs about unnecessary object copies with auto

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Thanks, @Manna, these changes look good to me! Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:175 SmallVector Bases; -for (const auto BaseS

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

tahonermann added inline comments. Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:304 +static_assert('\u{9}' == (char)1, ""); // expected-error {{failed}} \ + // expected-note {{evaluates to ''\t' (0x09, 9) == '' (0x01, 1)'}} +static

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

tahonermann updated this revision to Diff 556604. tahonermann added a comment. Moved the added release note to the correct section. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64087/new/ https://reviews.llvm.org/D64087 Files: clang/docs/Releas

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

tahonermann added a comment. @Endill, thank you for the reminder about this old patch! @cor3ntin, I added an additional test and updated a few other recently (relative to the original patch!) added tests. Would you be so kind as to give this another quick review? Repository: rG LLVM Github

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

tahonermann updated this revision to Diff 556592. tahonermann edited the summary of this revision. tahonermann added a comment. Rebased patch uploaded. This retains the same code change, but includes additional test updates for tests added since the first patch was submitted, as well as an addit

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

tahonermann added inline comments. Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:304 +static_assert('\u{9}' == (char)1, ""); // expected-error {{failed}} \ + // expected-note {{evaluates to ''\t' (0x09, 9) == '' (0x01, 1)'}} +static

[PATCH] D159345: [Clang] Handle non-ASCII after line splicing

tahonermann accepted this revision. tahonermann added a comment. Even better than I asked for. I held back on suggesting the change of `Tok` to `Result` to match `tryConsumeIdentifierUCN()`, but you made that change anyway! You must have read my mind! :) Repository: rG LLVM Github Monorepo

[PATCH] D155610: [Clang][Sema] Fix display of characters on static assertion failure

tahonermann added inline comments. Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:304 +static_assert('\u{9}' == (char)1, ""); // expected-error {{failed}} \ + // expected-note {{evaluates to ''\t' (0x09, 9) == '' (0x01, 1)'}} +static

[PATCH] D159345: [Clang] Handle non-ASCII after line splicing

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. This looks good to me modulo a couple of nits. Comment at: clang/include/clang/Lex/Lexer.h:806-807 /// Try to consume an identifier character encoded in UTF-8.

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6658db5e5014: [clang] Fix timing of propagation of MSInheritanceAttr for template… (authored by tahonermann). Repository: rG LLVM Github Monorepo

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

tahonermann updated this revision to Diff 555076. tahonermann added a comment. Added a release note. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158869/new/ https://reviews.llvm.org/D158869 Files: clang/docs/ReleaseNotes.rst clang/lib/Sema/S

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Thanks, Soumi, looks good to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158293/new/ https://reviews.llvm.org/D158293 ___

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

tahonermann added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:494-495 ++NumSkipped; assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?"); + assert(CurLexer && "Invalid lexer value"); Would you mind splitting out all of th

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

tahonermann updated this revision to Diff 554413. tahonermann marked an inline comment as done. tahonermann edited the summary of this revision. tahonermann added a comment. Address prior review feedback. This update also modifies the propagation of the `MSInheritanceAttr` attribute for explicit

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

tahonermann marked 2 inline comments as done. tahonermann added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:10110-10111 +// propagated to the new node prior to instantiation. +if (PrevDecl && PrevDecl->hasAttr()) { + Specialization->addAttr(PrevDecl-

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

tahonermann requested changes to this revision. tahonermann added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Lex/PPDirectives.cpp:494-495 ++NumSkipped; - assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?"); + asser

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

tahonermann added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:10112 + Specialization->addAttr(PrevDecl->getAttr()); + Consumer.AssignInheritanceModel(Specialization); +} erichkeane wrote: > tahonermann wrote: > > I am concerned that

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

tahonermann added reviewers: rnk, zequanwu. tahonermann added a comment. Adding Erich as attributes owner, and Reid and Zequan for MS ABI. This patch slightly modifies the changes made via D94646 and committed as rG4fffbc150cca1638051b8ad2a20f4b8240df0869

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

tahonermann created this revision. tahonermann added reviewers: erichkeane, aaron.ballman. Herald added a project: All. tahonermann requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This change addresses two issues: 1. A failure to propagate

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

tahonermann added a comment. I think it would also be a good idea to exercise the test case for a target that lacks ifunc support to ensure that inter-TU calls work as expected. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158666/new/ https://reviews.llvm.org/D158666

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

tahonermann added a comment. A couple of observations with regard to compatibility: gcc,, at least by default, emits the TC implementations as local functions, the resolver as a weak global function, and the undecorated name as an ifunc. When only a TC declaration (not a definition) is present,

[PATCH] D158522: [NFC][CLANG] Fix static analyzer bugs about large copy by values

tahonermann accepted this revision. tahonermann added a comment. Looks fine to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158522/new/ https://reviews.llvm.org/D158522 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D158293: [NFC][Clang] Fix static code analyzer concern about null value dereference

tahonermann requested changes to this revision. tahonermann added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Lex/PPDirectives.cpp:494 ++NumSkipped; assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?"); --

[PATCH] D158372: [Clang] Treat invalid UDL as two tokens

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. Given Richard's comments, it seems that changes are needed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158372/new/ https://reviews.llvm.org/D158372 ___

[PATCH] D158488: [NFC] Initialize member pointers to nullptr.

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Looks good to me. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:934 // Return value of the runtime offloading call. - Value *Return; + Value *Return =

[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

tahonermann added inline comments. Comment at: clang/lib/AST/Interp/InterpBlock.cpp:94-95 DeadBlock::DeadBlock(DeadBlock *&Root, Block *Blk) -: Root(Root), B(Blk->Desc, Blk->IsStatic, Blk->IsExtern, /*isDead=*/true) { +: Root(Root), B(Blk->Desc, Blk->IsExtern, Blk->IsS

[PATCH] D158488: [NFC] Initialize member pointers to nullptr.

tahonermann added inline comments. Comment at: clang/include/clang/AST/Stmt.h:2606-2607 enum { INIT, CONDVAR, COND, INC, BODY, END_EXPR }; - Stmt* SubExprs[END_EXPR]; // SubExprs[INIT] is an expression or declstmt. + Stmt *SubExprs[END_EXPR] = { + nullptr}; // SubExprs[

[PATCH] D155776: [NFC] Add checks for self-assignment.

tahonermann accepted this revision. tahonermann added a comment. Thank you for following up, Sindhu! Looks good! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155776/new/ https://reviews.llvm.org/D155776 ___ cfe-commits mailing list cfe-commi

[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

tahonermann added a comment. > To me, an important metric of a quality patch is if the title and the summary > describe why we have this patch, and what we do about it. @steakhal, I'll work with the people submitting these patches to add more context to the title, summary, and commit comments.

[PATCH] D158293: [NFC][CLANG] Fix potential dereferencing of null return values

tahonermann added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:2009-2010 (Line.MightBeFunctionDecl || Line.InPPDirective) && - Current.NestingLevel == 0 && + Current.NestingLevel == 0 && Current.Previous &&

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

tahonermann added inline comments. Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9 +// Note: We must use the real path here, because the logic to detect case +// mismatch relies on resolving the real path and checking that casing differs. +// If we use %t and we a

[PATCH] D157989: [NFC] Initialize pointer field

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Looks good to me. I see that `OpenMPIRBuilder::emitTargetKernel()` can return without having assigned the `Return` parameter. Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D157888: [NFC][Clang] Fix static analyzer concern about null value dereference

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Looks good to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157888/new/ https://reviews.llvm.org/D157888 ___ cfe-commits mai

[PATCH] D157554: [NFC][Clang] Fix static analyzer concern about null pointer dereference

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Looks good to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157554/new/ https://reviews.llvm.org/D157554 ___ cfe-commits mai

[PATCH] D157885: [NFC][Clang] Fix static analyzer concern about null value derefence

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Looks fine to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157885/new/ https://reviews.llvm.org/D157885 ___ cfe-commits mai

[PATCH] D155081: Specify the developer policy around links to external resources

tahonermann added a comment. This looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155081/new/ https://reviews.llvm.org/D155081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/list

[PATCH] D157454: [NFC][Clang] Fix static analyzer concern about null value dereference

tahonermann accepted this revision. tahonermann added a comment. Looks good to me. Thanks, Elizabeth! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157454/new/ https://reviews.llvm.org/D157454 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D157454: [NFC][Clang] Fix static analyzer concern about null value dereference

tahonermann added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:222-223 Selector Sel = MethodWithObjects->getSelector(); - QualType ResultType = E->getType(); - const ObjCObjectPointerType *InterfacePointerType -= ResultType->getAsObjCInterfacePointerType();

[PATCH] D157454: [NFC][Clang] Fix static analyzer concern about null value dereference

tahonermann added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:222-223 Selector Sel = MethodWithObjects->getSelector(); - QualType ResultType = E->getType(); - const ObjCObjectPointerType *InterfacePointerType -= ResultType->getAsObjCInterfacePointerType();

[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

tahonermann requested changes to this revision. tahonermann added a comment. I agree with the analysis done by @steakhal. The static analyzer being used (which I'm not allowed to name) is being too noisy in these cases; it complains about every case where an object of class type is copied regard

[PATCH] D157118: [NFC][Clang] Fix static analyzer concerns

tahonermann accepted this revision. tahonermann added a comment. I think this change is good as is, but I added some additional thoughts to Aaron's earlier comment. Comment at: clang/lib/AST/StmtPrinter.cpp:178 void StmtPrinter::PrintRawCompoundStmt(CompoundStmt *Node) { + a

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

tahonermann added a comment. > @tahonermann are all your comments addressed at this point? Apologies for the late response; I was on vacation last week. No. I'm still skeptical that there is ever a desire to expand substitute drives. Comment at: clang/test/Lexer/case-insensit

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. > You are absolutely right, -fno-coroutines would totally work for us, had it > been available. Good. Gcc handles `-fcoroutines` and `-fno-coroutines` as I would expect (h

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

tahonermann added a comment. At first, it sounded to me like the option that would best suit Google's needs is `-fno-coroutines`. However: - I see that Google does have some limited use of coroutines; perhaps that could be enabled on a per-TU or project basis by the build system? However... - T

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. I'm happy with this. I noted one last rename suggestion to maintain consistency, but am accepting. Please give Corentin a final chance to raise any concerns. Thank you for being so r

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1981 + // parsed yet). + assert(getLangOpts().MicrosoftExt); + RIscRIpt wrote: > I think I should remove this assertion so this function would be usable > without MS ext, on the other h

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann added a comment. I think I'm happy with this. I noted two code changes that I think are no longer needed and offered some final suggestions on some names. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:118-120 +def ext_expand_predef_ms : ExtWarn< + "

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:2010-2013 +OS << "R\"EFLPM(" + << PredefinedExpr::ComputeName(getPredefinedExprKind(Tok.getKind()), + currentDecl) + << ")EFLPM\""; cor

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

tahonermann added a comment. > Marking comments as resolved per my reply. I'm not sure if that's best > practice! Yes, that is fine. Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9 +// Note: We must use the real path here, because the logic to detect case +//

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann added inline comments. Comment at: clang/test/Sema/ms_predefined_expr.cpp:156 +static_assert(__FUNCTION__ ""_len == 14); // expected-warning{{string literal concatenation of predefined identifier '__FUNCTION__' is a Microsoft extension}} } Here

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:2010-2013 +OS << "R\"EFLPM(" + << PredefinedExpr::ComputeName(getPredefinedExprKind(Tok.getKind()), + currentDecl) + << ")EFLPM\""; RIs

[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

tahonermann added inline comments. Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9 +// Note: We must use the real path here, because the logic to detect case +// mismatch relies on resolving the real path and checking that casing differs. +// If we use %t and we a

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. I think this is looking good. The only big thing I noticed is some code that looks like it should have been removed with the last set of changes. Otherwise, just some minor

[PATCH] D155846: [NFC][clang] Fix static analyzer concerns

tahonermann added a comment. > I agree. I'll try to isolate a reproducible test case and report it to the > vendor. Done. I'm ambivalent with regard to keeping or discarding the proposed change, but the static analysis issue should be triaged as a false positive. Repository: rG LLVM Github

[PATCH] D155776: [NFC] Add checks for self-assignment.

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. On second thought, and after having read https://stackoverflow.com/a/9322542/11634221, I think this change is fine. There are legitimate scenarios for which self move assignment shou

[PATCH] D155842: [NFC][clang] Fix static analyzer concerns

tahonermann accepted this revision. tahonermann added a comment. This is a good change since the default copy constructor would do bad things when `ownsOutputFile` is true. The copy assignment operator is already implicitly deleted because of the presence of a data member of reference type, but

[PATCH] D155846: [NFC][clang] Fix static analyzer concerns

tahonermann added a comment. > there's no bug being fixed which makes me think this should go back to the > static analysis vendor to report this as a false positive. I agree. I'll try to isolate a reproducible test case and report it to the vendor. Repository: rG LLVM Github Monorepo CHAN

[PATCH] D155849: [NFC][clang] Fix static analyzer concerns

tahonermann accepted this revision. tahonermann added a comment. Looks good; the default implementations would definitely do the wrong thing, so this is a good find. My suggested edit is just to add a blank line. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1823-1826 + OMPT

[PATCH] D155776: [NFC] Add checks for self-assignment.

tahonermann added a comment. Hmm, it seems exceptionally unlikely for a move assignment operator to be invoked on an object with itself as the source argument. That would require an explicit use of `std::move()` as in: Value x = ...; x = std::move(x); Rather than protecting against that an

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

tahonermann added a comment. It is unclear to me why/when we would ever want the substitute drive expansion; the modified tests aren't very elucidating. My naive expectation is that we effectively want to restore the Python 3.7 behavior. Comment at: clang/test/ExtractAPI/rela

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann added inline comments. Comment at: clang/include/clang/Basic/TokenKinds.h:87-93 +/// Return true if this token is a predefined macro +/// unexpandable by MSVC preprocessor. +inline bool isUnexpandableMsMacro(TokenKind K) { + return K == tok::kw___FUNCTION__ || K == t

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann requested changes to this revision. tahonermann added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Basic/TokenKinds.h:87-93 +/// Return true if this token is a predefined macro +/// unexpandable by MSVC preprocessor.

[PATCH] D155610: [Clang][ExprConstant] Print integer instead of character on static assertion failure

tahonermann added inline comments. Comment at: clang/test/Lexer/cxx1z-trigraphs.cpp:24 // expected-error@11 {{}} expected-warning@11 {{trigraph ignored}} -// expected-error@13 {{failed}} expected-warning@13 {{trigraph ignored}} expected-note@13 {{evaluates to ''?' == '#''}} +//

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

tahonermann added a comment. >> In D154130#4486898 , @tahonermann >> wrote: >> >>> 95% of the %>t are around clang modulemap files, because that code resolves >>> real paths in C++ by design, so I can't avoid it. >> >> Can you link to evidence that this

[PATCH] D153914: [clang-cl] Enable concatenation of predefined identifiers

tahonermann requested changes to this revision. tahonermann added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:118-120 +def ext_concat_predef_ms : ExtWarn< + "concatenation of predefined identifier '

[PATCH] D153621: [Clang] Correctly handle $, @, and ` when represented as UCN

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. > I'll probably merge that EoW unless you scream! Why wait? :) Looks good! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153621/new/

[PATCH] D153892: [NFC] Initialize class member pointers to nullptr.

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Thanks, Sindhu! This looks good to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153892/new/ https://reviews.llvm.org/D153892 __

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

tahonermann added a comment. > 95% of the %>t are around clang modulemap files, because that code resolves > real paths in C++ by design, so I can't avoid it. Can you link to evidence that this behavior is by design? It isn't obvious to me why modulemap files would demand different behavior; es

[PATCH] D153418: Adding iconv support to CharSetConverter class

tahonermann added a subscriber: hubert.reinterpretcast. tahonermann added a comment. > Please correct me if I'm wrong, I'm not too familiar with icu4c, but I think > adding support for ICU would be the better long-term solution since it seems > to allow the same behaviour across different platfo

[PATCH] D153621: [Clang] Correctly handle $, @, and ` when represented as UCN

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. Changes look good; I added a number of suggested edits for minor issues. Comment at: clang/docs/ReleaseNotes.rst:203-204 +- Implemented `WG14 N3124

[PATCH] D153892: [NFC] Initialize class member pointers to nullptr.

tahonermann requested changes to this revision. tahonermann added inline comments. This revision now requires changes to proceed. Herald added a subscriber: wangpc. Comment at: clang/lib/Frontend/ASTConsumers.cpp:192-198 + bool HandleTopLevelDecl(DeclGroupRef D) override {

[PATCH] D153926: [NFC] Initialize class member pointers to nullptr.

tahonermann accepted this revision. tahonermann added a comment. Herald added a subscriber: wangpc. These changes look find to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153926/new/ https://reviews.llvm.org/D153926 ___ cfe-commits mail

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

tahonermann accepted this revision. tahonermann added a comment. These changes look fine to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153589/new/ https://reviews.llvm.org/D153589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. Sorry for the long delay reviewing again. I noted some mostly minor issues. I'm on vacation next week, but I'll try to watch for updates. I'm still concerned that the chang

[PATCH] D151606: [NFC][CLANG] Fix Static Code Analyzer Concerns with bad bit right shift operation in getNVPTXLaneID()

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. The static analysis results look suspect since there does not appear to be a way for `Log2_32` to return a value larger than 31. Regardless, the change looks safe to me; `GV_Warp_Siz

[PATCH] D150931: [NFC][Clang] Fix Static Code Analysis Concerns with copy without assign

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. These changes look fine to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150931/new/ https://reviews.llvm.org/D150931 ___ cf

[PATCH] D150744: [NFC][CLANG] Fix uninitialized scalar field issues

tahonermann accepted this revision. tahonermann added a comment. > is it ok to land this patch? No objection from me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150744/new/ https://reviews.llvm.org/D150744 ___ cfe-commits mailing list cfe

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. > They are NFC, it's just not 100% only a refactoring, since it adds the new > ASCII-only case. Ok, thanks. Changes look good. I noticed one comment that appears to be incorrect; pl

[PATCH] D128059: [Clang] Add a warning on invalid UTF-8 in comments.

tahonermann added a comment. @cor3ntin, sorry for failing to keep up with reviews; I know this has already been committed. I did spot a couple of typos should you feel inclined to address them. Comment at: clang/lib/Lex/Lexer.cpp:2695 + // diagnostic only once per entire ill

[PATCH] D146924: [clang] Add support for dollar sign in ud_suffix of numeric constants

tahonermann added a comment. > I wrote a paper https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3124.pdf It appears you back ported that paper from the future: "Date: 2024-22-04" :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146924/new/ https:

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tahonermann added a comment. The changes look good to me. I suggested two minor edits to align with parameter name changes. The summary states that the changes are not quite NFC. In that case, we would ideally have a test that demonstrates the changed behavior. Would adding such a test be chal

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

tahonermann added a comment. I'm late to review and can no longer stamp an approval on this, but I'll note for the historical record that the changes look good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150913/new/ https://reviews.llvm.o

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

tahonermann added a comment. This is looking good to me. I added a few comments seeking clarification. I'm concerned about the changes to the existing calls to `getFloatingTypeOrder()`. The changes look like they will preserve prior behavior for standard types just fine, but I'm not sure whethe

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

tahonermann added a comment. > @tahonermann Just a quick note - I've made the changes you initially > suggested in the RFC. When you get a chance, could you have another look? Yes, I hope to today. If not, I'll make it a priority for tomorrow. CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Thanks @Manna, this looks good to me now! Comment at: clang/include/clang/Sema/ParsedAttr.h:705-707 + // The move assignment operator is defined as deleted pending

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. Splitting this into two patches (one to do the renames, another to perform the other changes) would simplify review. Comment at: clang/lib/Frontend/TextD

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

tahonermann added a comment. One last minor request and then I'm happy with this. Comment at: clang/include/clang/Sema/ParsedAttr.h:705-707 + // The move assignment operator is defined as deleted pending further + // motivation. + AttributePool &operator=(AttributePool &&poo

[PATCH] D150744: [NFC][CLANG] Fix uninitialized scalar field issues found by Coverity

tahonermann added inline comments. Comment at: clang/include/clang/Parse/Parser.h:1190 class ParseScopeFlags { Scope *CurScope; +unsigned OldFlags = 0; shafik wrote: > @tahonermann I feel like we should have a default member initializer for any > mem

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

tahonermann added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1790 SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default; +SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete; ~SemaDiagnosticBuilder();

[PATCH] D150744: [NFC][CLANG] Fix uninitialized scalar field issues found by Coverity

tahonermann added a comment. I'm not opposed to these changes, but please note that these changes mean is will no longer be possible to use ubsan to discover when these data members are used before having been assigned an appropriate value. That is only a concern when an appropriate value would

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. I added a few comments and suggested edits, but this is mostly looking good. Comment at: clang/include/clang/Sema/Sema.h:1790 SemaDiagnosticBuilder(c

[PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. > I do wonder if we need two bfloat implementations, but for that I'll leave a > comment on D149573 . Given the discussions occurring in D

[PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. Thanks for all the updates @codemzs! I'm going to go ahead and accept. But please wait a few days for recently subscribed folks to have a chance to comment before landing this. Rep

  1   2   3   4   >