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

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. Looks like a great improvement to the policy, thank you all for sorting through this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155081/new/ https://reviews.llvm.org/D155081

[PATCH] D57896: Variable names rule

lattner added a comment. I would also love to see this conceptually, but think it will be pretty polarizing in the community. It is worth another RFC thread before investing much time in it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D155509: Revert "Remove rdar links; NFC"

lattner accepted this revision. lattner added a comment. Thank you, this should have been reverted immediately when concerns were raised. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155509/new/ https://reviews.llvm.org/D155509

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

lattner requested changes to this revision. lattner added a comment. This revision now requires changes to proceed. Thank you so much for raising this issue Aaron. Documenting the policy in the developer policy is totally the right thing to do given this cross-cuts individual subprojects, but I

[PATCH] D150997: [llvm] Split out DenseMapInfo specialization

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. Awesome, this makes a lot of sense and is a great pattern to make these more modular. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150997/new/ https://reviews.llvm.org/D150997

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

lattner added a comment. Got it this time, sorry for the confusion! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139705/new/ https://reviews.llvm.org/D139705 ___ cfe-commits mailing list

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

lattner added a comment. I'm pretty sure I'm on top of commit access requests now, plz let me know if I missed you or something! Could be spam filter or who knows what Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139705/new/

[PATCH] D137960: [Lexer] Speedup LexTokenInternal

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. Nice! I'd love someone else's eyes on this, but that's a great speedup! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137960/new/

[PATCH] D131919: Move googletest to the third-party directory

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. I didn't review the patch in detail, but +1 this is a great step forward to reorganize the repo! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

lattner added a comment. I haven't reviewed this in detail, but oh my I love LOVE LOVE this! `llvm::scc_traversal(x)` is so nice, great work! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131448/new/ https://reviews.llvm.org/D131448

[PATCH] D126068: [llvm][clang][bolt][NFC] Use llvm::less_first() when applicable

lattner added a comment. This patch LGTM other than the apparent build breakage Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126068/new/ https://reviews.llvm.org/D126068 ___ cfe-commits mailing list

[PATCH] D126324: [clang] Allow const variables with weak attribute to be overridden

lattner added a comment. I'm not a competent reviewer for the patch, but +1 on aligning with GCC behavior here. I don't recall the motivation for the original patch (apologies again for the terrible commit message). This approach makes a lot of sense to me. Repository: rG LLVM Github

[PATCH] D125557: [APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf

lattner added inline comments. Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2884 if (IntrinsicID == Intrinsic::smul_fix_sat) { - APInt Max = APInt::getSignedMaxValue(Width).sextOrSelf(ExtendedWidth); - APInt Min =

[PATCH] D125557: [APInt] Remove all uses of zextOrSelf, sextOrSelf and truncOrSelf

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. nice cleanup! Comment at: llvm/lib/Analysis/ConstantFolding.cpp:2884 if (IntrinsicID == Intrinsic::smul_fix_sat) { - APInt Max =

[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

lattner edited reviewers, added: rsmith; removed: lattner. lattner added a comment. I'm not a competent reviewer for this, Richard can you recommend someone? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125402/new/

[PATCH] D123957: Update the developer policy to mention release notes

lattner accepted this revision. lattner added a comment. Nice, LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123957/new/ https://reviews.llvm.org/D123957 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D123957: Update the developer policy to mention release notes

lattner added a comment. Also, when this lands, we should post on the forum about it. Every change to the developer policy warrants broader visibility than just a phab discussion IMO. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123957/new/ https://reviews.llvm.org/D123957

[PATCH] D123957: Update the developer policy to mention release notes

lattner added a comment. This is awesome, I agree completely we should curate release notes better. That said, I think this should make it more clear that there is a "difference in kind" between user-facing tools like clang/lldb etc and other libraries in LLVM. We don't want release note

[PATCH] D123064: [Clang][C++23] P2071 Named universal character escapes

lattner added a comment. Yeah, we should discuss this, thanks for raising this Aaron. I'm not sure exactly what is being pulled in: @cor3ntin can you please email a summary of the situation to bo...@llvm.org and we'll discuss it and run it by Heather as needed? Thanks -Chris Repository:

[PATCH] D120713: [clangd] Make dexp command line options sticky

lattner resigned from this revision. lattner added a comment. Herald added a project: All. I'm not a competent reviewer for clangd Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120713/new/ https://reviews.llvm.org/D120713

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

lattner added a comment. Thank you so much Carlos, I hope they can come to see the value of having first class support for this in LLVM/Clang. I really appreciate you pushing for this, -Chris CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112730/new/ https://reviews.llvm.org/D112730

[PATCH] D112730: [clang-tidy] Add AUTOSAR module

lattner added a comment. Hi all, I'm commenting on this based on my personal opinion, I don't speak for the LLVM board or anyone else. I am also not a lawyer :) This isn’t a clear cut case (as is typical!). LLVM's approach on patents protection revolves primarily around the terms in the

[PATCH] D113641: [llvm] Add a SFINAE template parameter to DenseMapInfo

lattner added a comment. Oh, are you concerned about staging this in? If you want to stage it (add the includes now, remove them later or something), that is totally fine with me. Maybe I don't understand the impact correctly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D113641: [llvm] Add a SFINAE template parameter to DenseMapInfo

lattner added a comment. I think a few void's probably won't hurt anyone? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113641/new/ https://reviews.llvm.org/D113641 ___ cfe-commits mailing list

[PATCH] D113641: [llvm] Add a SFINAE template parameter to DenseMapInfo

lattner accepted this revision. lattner added a comment. Herald added a subscriber: sdasgup3. Nice, I'm very excited about this - this will allow a lot of cleanups. Thank you for doing this! Comment at: llvm/include/llvm/ADT/Hashing.h:59 namespace llvm { -template struct

[PATCH] D113080: [Support] Improve Caching conformance with Support library behavior

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. nice, thank you for improving this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113080/new/ https://reviews.llvm.org/D113080

[PATCH] D111049: [Support] Change fatal_error_handler_t to take a const char* instead of std::string

lattner accepted this revision. lattner added a comment. This is really great, thank you for doing this. I'd love to see reduced out of headers! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111049/new/ https://reviews.llvm.org/D111049

[PATCH] D110808: [APInt] Stop using soft-deprecated constructors and methods in clang. NFC.

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. Thank you Jay! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110808/new/ https://reviews.llvm.org/D110808

[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

lattner resigned from this revision. lattner edited reviewers, added: rsmith; removed: lattner. lattner added a comment. I'm not sure who the best person is to review this, but it isn't me anymore sadly. Richard, can you recommend someone? Comment at:

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

lattner added a comment. > What is a "keep constructor"? Good question, I'm not sure. I think I meant to say "key constructors". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109483/new/ https://reviews.llvm.org/D109483

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

lattner added a comment. I'll take care of the DAG.getAllOnesConstant change, but i'd appreciate it if you could look at the NOT cases. Running tests on the DAG.getAllOnesConstant patch now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

lattner added a comment. Thank you for the detailed review Craig! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109483/new/ https://reviews.llvm.org/D109483 ___ cfe-commits mailing list

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

lattner added inline comments. Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3243 "Don't know how to expand this subtraction!"); -Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1), -

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

lattner marked an inline comment as done. lattner added inline comments. Comment at: llvm/include/llvm/ADT/APInt.h:384 /// value for the APInt's bit width. bool isMaxValue() const { return isAllOnesValue(); } craig.topper wrote: > isAllOnes()? Yep, good

[PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

lattner added a comment. This patch has a lot of noise, the important part is APInt.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109483/new/ https://reviews.llvm.org/D109483 ___ cfe-commits mailing

[PATCH] D107528: [llvm][clang][NFC] updates inline licence info

lattner accepted this revision. lattner added a comment. Thank you for catching this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107528/new/ https://reviews.llvm.org/D107528 ___ cfe-commits mailing

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

lattner added a comment. > @lattner, thanks for the help. In this case, the real question is whether > there's any use case for !srcloc that involves writing it out into a bitcode > or IR file and then having a separate instance of clang load it back in again. > I think that no such case can

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

lattner added a comment. > cat f.c void x() { asm("bogus"); } > clang f.c f.c:3:7: error: invalid instruction mnemonic 'bogus' asm("bogus"); ^ :1:2: note: instantiated into assembly here bogus ^ 1 error generated. Repository: rG LLVM

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

lattner added a comment. The usecase for this is to get good diagnostics out of the integrated assembler. Try doing something like: `asm("bogus");`, GCC would produce an error pointing to the generated .s file. Clang should point to the C file (including macros expanded etc). Repository:

[PATCH] D104819: [ADT] Rename StringRef case insensitive methods for clarity

lattner accepted this revision. lattner added a comment. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104819/new/ https://reviews.llvm.org/D104819 ___ cfe-commits mailing list

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

lattner added inline comments. Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600 + if (RHS.data() == getEmptyKey().data()) +return LHS.data() == getEmptyKey().data(); + if (RHS.data() == getTombstoneKey().data()) +return LHS.data() ==

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

lattner added inline comments. Comment at: llvm/include/llvm/ADT/ArrayRef.h:595-600 + if (RHS.data() == getEmptyKey().data()) +return LHS.data() == getEmptyKey().data(); + if (RHS.data() == getTombstoneKey().data()) +return LHS.data() ==

[PATCH] D103491: [ADT] Move DenseMapInfo for ArrayRef/StringRef into respective headers (NFC)

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. > This required adding quite a few additional includes, as many files were > relying on various things pulled in by ArrayRef.h. This is a _good_ thing btw! Thank you for driving this!

[PATCH] D97320: Use a fast path when initializing LineOffsetMapping

lattner added a comment. Nice, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97320/new/ https://reviews.llvm.org/D97320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D92269: [TableGen] Eliminate the 'code' type

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. Woot! Go Paul! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92269/new/ https://reviews.llvm.org/D92269 ___ cfe-commits mailing list

[PATCH] D92269: [TableGen] Eliminate the 'code' type

lattner added a comment. Oh ok, well I have no problem keeping it around in searchable tables, I misread this as making that more general. jrtc27, we had a long conversation about this. We'd like to eliminate the Code type as a way to simplify the internals of tblgen. It is a distinction

[PATCH] D92269: [TableGen] Eliminate the 'code' type

lattner added a comment. Ah, I thought TypeOf_xxx was a transitionary thing that we'd drop over time (to reduce complexity). If that's the case, I'd recommend not documenting it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92269/new/

[PATCH] D89893: [Clang] [TableGen] Clean up !if(!eq(boolean, 1) and related booleans

lattner added a comment. Oops, thanks for the head's up @dblaikie ! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89893/new/ https://reviews.llvm.org/D89893 ___ cfe-commits mailing list

[PATCH] D36836: [clang-tidy] Implement sonarsource-function-cognitive-complexity check

lattner added a comment. Hi all, The LLVM foundation board discussed this offline -- it looks like the contributor is a bit confused about the LLVM project policies towards copyright, license, patent stuff etc. I really appreciate the dilligence here to capture the ideas that went into this

[PATCH] D81728: [InstCombine] Add target-specific inst combining

lattner resigned from this revision. lattner added a comment. Please don't consider me a blocker on this patch, thank you for pushing on it! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81728/new/ https://reviews.llvm.org/D81728

[PATCH] D81728: [InstCombine] Add target-specific inst combining

lattner requested changes to this revision. lattner added a comment. This revision now requires changes to proceed. This looks like a great direction, but please make sure to minimize public implementation details. We don't want the vast majority of instcombine to be visible outside of its

[PATCH] D82259: Deprecate error prone temporary directory APIs

lattner added a comment. Hi Dave, This is doing two things - changing the behavior of clang (where it stores its module cache) and deprecating an API. I'd recommend we start by focusing on whether the first part is the right thing to do, and just remove the API if this is the only client of

[PATCH] D78870: [SourceMgr] Tidy up the SourceMgr header file to include less stuff.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG919dcc7f6858: [SourceMgr] Tidy up the SourceMgr header file to include less stuff. (authored by lattner). Changed prior

[PATCH] D78870: [SourceMgr] Tidy up the SourceMgr header file to include less stuff.

lattner updated this revision to Diff 260140. lattner added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Clang depended on the transitive include, update it! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

This revision was automatically updated to reflect the committed changes. Closed by commit rG39c9c12b76da: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet. (authored by lattner). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

lattner added a comment. Thank you for the review Reid! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78273/new/ https://reviews.llvm.org/D78273 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

lattner added a comment. Hey @maskray, yes StringSet/StringMap's entries are separately allocated and the iterators/pointers are stable. The strings are held in each entry. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78273/new/

[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

lattner created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. MaskRay added inline comments. Comment at: clang-tools-extra/modularize/PreprocessorTracker.cpp:912 // Lookup/add string. - StringHandle addString(llvm::StringRef Str) {

[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

lattner added a comment. I don't really understand what this is getting at, so I'd recommend rewording this a bit for clarity. Would something along the lines of this capture the intended meaning?: If there is likely to be uncertainty, you should default to getting a patch reviewed prior to

[PATCH] D75169: [ARM] Enforcing calling convention for half-precision FP arguments and returns for big-endian AArch32

lattner resigned from this revision. lattner added a comment. I'm not a qualified reviewer for this at this point. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75169/new/ https://reviews.llvm.org/D75169

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

lattner added a comment. I'd be happy to help fix that problem. Please take a look at the llvm developer policy. :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74801/new/ https://reviews.llvm.org/D74801 ___ cfe-commits mailing list

[PATCH] D74801: [ADT][NFC] SCCIterator: Change hasLoop() to hasCycle()

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

[PATCH] D69868: Allow "callbr" to return non-void values

lattner accepted this revision. lattner added a comment. I'm super excited to see this progress towards supporting 'asm goto' with results! Great work! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69868/new/ https://reviews.llvm.org/D69868

[PATCH] D57896: Variable names rule

lattner added a comment. In D57896#1434877 , @Charusso wrote: > static Optional > getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) { > //... > > if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) { > if (const auto *VD =

[PATCH] D57896: Variable names rule

lattner added a comment. I can understand Zach's position here, but LLDB has historically never conformed to the general LLVM naming or other conventions due to its heritage. It should not be taken as precedent that the rest of the project should follow. In any case, I think that it is best

[PATCH] D57896: Variable names rule

lattner added a comment. > Changed recommendation for acronyms from lower case to upper case, as > suggested by several responses to the RFC. I haven't been following the discussion closely - why is this the preferred direction? I don't think that things like "Basicblock *bb" or "MachineInstr

[PATCH] D57896: Variable names rule

lattner accepted this revision. lattner added a comment. This revision is now accepted and ready to land. I am very much +1 on this. That said, this isn't the sort of thing we just use patch review for. Please agitate a robust discussion about this on llvm-dev. :-) Repository: rG LLVM