[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

dblaikie added a comment. I guess my main question is: What's the motivation for implementing this? Do you have a need/use for this? (it doesn't seem to be motivated by GCC compatibility - as discussed, looks like we're diverging in a bunch of ways from their behavior and the argument made

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

dblaikie added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149182/new/ https://reviews.llvm.org/D149182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D149380: [clang] Add -Wunused-result-always warning

dblaikie added a comment. Seems more stylistic/unlikely to meet the diagnostic bar (certtainly couldn't be on-by-default for instance) for clang to me (but could go into something like clang-tidy), at least. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D148769: Split out `CodeGenTypes` from `CodeGen` for LLT/MVT

dblaikie accepted this revision. dblaikie added a comment. I wouldn't mind a follow-up commit that creates a new/separate directory (llvm/{lib,include/llvm}/CodeGenTypes) but that's probably a lot of unnecessary churn too, so I can appreciate the argument against it - if folks reckon it's

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

dblaikie added a comment. > probably too much, but then I wonder "how do we make Fortify work?". (I'm still sort of on the side of "if the compile-time costs to get more verbose diagnostics is high, we could have a low-quality default-on diagnostic and it could mention/recommend a

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

dblaikie added a comment. In D149193#4302981 , @scott.linder wrote: > I am OK to give LGTM, assuming the other reviewers don't still have > reservations? Some - planning to get to this next week. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

dblaikie added inline comments. Comment at: clang/lib/Driver/Driver.cpp:3884 + nullptr, getOpts().getOption(options::OPT_dumpdir), + Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-")); + Arg->claim(); would be nice to

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

dblaikie created this revision. dblaikie added a reviewer: aaron.ballman. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The packed attribute can still be useful in this case if the struct is then

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

dblaikie added a comment. In D147844#4293743 , @cjdb wrote: > In D147844#4293693 , @dblaikie > wrote: > >>> I think some of the cases are ambiguous while others are not. >> >> Data would be good to have - if

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

dblaikie added a comment. > I think some of the cases are ambiguous while others are not. Data would be good to have - if this assessment is true, we'd expect this to bear out in terms of bug finding, yeah? (that the cases you find to be ambiguous turn up as real bugs with some significant

[PATCH] D148444: [clang-tidy] Prevent `llvmlibc-inline-function-decl` triggering on lambdas

dblaikie added a comment. > Additionally, lambdas are always going to have internal linkage so they will > not differ accross TUs. This isn't true - if a lambda appears in a header in any way, it probably has linkage the same as an inline function, not internal. (eg: a lambda inside an inline

[PATCH] D148851: Disable llvm-symbolizer on some of the driver tests that are timing out

dblaikie added a comment. > I think we probably should add LLVM_DISABLE_SYMBOLIZATION=1 to the lit level, > not in individual tests. Though I'm not sure how to do that in a way that it doesn't apply to test that are genuinely failing, where a symbolized backtrace is helpful/important/exactly

[PATCH] D148851: Disable llvm-symbolizer on some of the driver tests that are timing out

dblaikie added a comment. I'm OK with this, though I wouldn't mind a more robust/general solution to this - especially all gunit death tests have this problem too - they crash and spend significant time symbolizing, I think? So it'd be great if we could find some way to run those with

[PATCH] D141310: [clang] add -Wcompare-function-pointers

dblaikie added a comment. FWIW I think it's still worth some data from applying this to a broad codebase like Chromium/wherever it's planned to be used - whether it's practical to make a codebase clean of this warning, what sort of challenges arise, whether we should consider/need some way to

[PATCH] D146595: [clang] Add "debug_trampoline" attribute

dblaikie added a comment. In D146595#4284268 , @aaron.ballman wrote: > In D146595#4284220 , @dblaikie > wrote: > >> In D146595#4281710 , @aprantl >> wrote: >> >>> Now

[PATCH] D146595: [clang] Add "debug_trampoline" attribute

dblaikie added a comment. In D146595#4281710 , @aprantl wrote: > In D146595#4278361 , @dblaikie > wrote: > >> In D146595#4235340 , @augusto2112 >> wrote: >> >>> In

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

dblaikie added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118 + if (Packed) +UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign); UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign); rjmccall

[PATCH] D146595: [clang] Add "debug_trampoline" attribute

dblaikie added a comment. In D146595#4235340 , @augusto2112 wrote: > In D146595#4235333 , @aprantl wrote: > >> I hope I'm not kicking off a long bike-shedding thread, but I would propose >> to either call the

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

dblaikie added a comment. > we should probably warn about all inconsistent references to the same > variable within a single statement. +1 here, that that seems like the suitable generalization/unrelated to the use in/restriction to assignments Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

This revision was automatically updated to reflect the committed changes. Closed by commit rGe9c9db34a9b0: PR58819: Correct linkage and mangling of lambdas in inline static member… (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D146595: [clang] Add "transparent_stepping" attribute

dblaikie added a comment. Is there some place (bug, discourse thread, etc) where the broader direction is discussed? I want to checkin on the design decisions/alternatives without fragmenting this across multiple reviews/losing context/etc? (specifically - this started out with the trampoline

[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

dblaikie added a comment. In D138247#4217189 , @efriedma wrote: > The relevant text of the current Itanium ABI (which was updated in > https://github.com/itanium-cxx-abi/cxx-abi/commit/d8e9d102c83f177970f0db6cc8bee170f2779bc1) > >> In the following

[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

dblaikie updated this revision to Diff 508847. dblaikie added a comment. Rename VariableTemplate classification to TemplatedVariable to match ABI wording Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138247/new/ https://reviews.llvm.org/D138247

[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

dblaikie updated this revision to Diff 508842. dblaikie added a comment. Remove `StaticDataMember` and classify static member variables of templates as variable templates Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138247/new/

[PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

dblaikie added inline comments. Comment at: clang/test/CodeGen/preferred_name.cpp:49 + +Foo> varFooInt; + Michael137 wrote: > dblaikie wrote: > > Michael137 wrote: > > > probinson wrote: > > > > This doesn't become `Foo` ? > > > The name stays as `Foo>` but

[PATCH] D146986: Downgrade reserved module identifier error into a warning

dblaikie added a comment. In D146986#4225192 , @aaron.ballman wrote: > In D146986#4225121 , @dblaikie > wrote: > >> From the discussion on the issue: >> >>> Do we want this loosening of the restriction to apply

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

dblaikie added a comment. In D146595#4225132 , @aprantl wrote: > In D146595#4225048 , @dblaikie > wrote: > >> I know this is a bit of a redirection/scope creep/etc - but I'd quite like >> to see a solution that

[PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

dblaikie added inline comments. Comment at: clang/test/CodeGen/preferred_name.cpp:49 + +Foo> varFooInt; + Michael137 wrote: > probinson wrote: > > This doesn't become `Foo` ? > The name stays as `Foo>` but the type of the template parameter > becomes

[PATCH] D146986: Downgrade reserved module identifier error into a warning

dblaikie added a comment. From the discussion on the issue: > Do we want this loosening of the restriction to apply to *only* `std` and the > same followed by a number, or to any reserved identifier used in a module? > e.g., > > module std; // error today, but will become a warning >

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

dblaikie added a comment. I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call

[PATCH] D146892: [documentation]Fixed Random Typo

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146892/new/ https://reviews.llvm.org/D146892

[PATCH] D146595: [clang] Add clang trampoline_mangled_target attribute

dblaikie added a comment. In D146595#4218040 , @augusto2112 wrote: What would happen if, instead, these trampolining functions were annotated nodebug? I guess then you wouldn't have the top level one as an entry point for the user, as

[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. In D146466#4218011 , @efriedma wrote: > I think the reason "recoverable" ubsan causes trouble is that it introduces > branches that subsequent optimizations can abuse. So without ubsan, we just > have an udiv instruction.

[PATCH] D146595: [clang] Add clang trampoline attribute

dblaikie added a comment. In D146595#4217855 , @augusto2112 wrote: >> Should there be some check that that symbol exists? (the current test uses >> the unmangled name "bar" for instance - which gives the wrong impression?) > > You mean as part of the

[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. (recoverable feels like a bit of a distraction here? recoverable just means you've asked ubsan not to trap/stop on failure - but to let the program continue and do whatever it would've done without the sanitizer enabled - sometimes that's crash/trap anyway, sometimes

[PATCH] D146595: [clang] Add clang trampoline attribute

dblaikie added a comment. >> What about function overloading & namespaces? Is the debugger expected to >> figure out which `bar` function the DWARF/user is referring to? > > The symbol name that's passed in the attribute is supposed to be the mangled > name, so there shouldn't be any ambiguity

[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

dblaikie added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138247/new/ https://reviews.llvm.org/D138247 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. In D146466#4213814 , @nickdesaulniers wrote: > In D146466#4207915 , @efriedma > wrote: > >> I'm concerned about the potential for false positives: >> >> - If a user doesn't mark a

[PATCH] D146595: [clang] Add clang trampoline attribute

dblaikie added a comment. So looking at the DWARF spec for this - I see what it's going for, but yeah, I wouldn't want to use a textual representation either at the source or bitcode level, ideally... - perhaps this should just be implemented in LLVM when something's being lowered to a

[PATCH] D146595: [clang] Add clang trampoline attribute

dblaikie added a comment. Yeah, this seems a bit burdensome for both the frontend (encoding more information, the author has to duplicate the name of the implementation function so there's a risk they get out of sync?) and the debugger (is the debugger expected to jump directly and not call

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (5/7)

dblaikie added inline comments. Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:712-714 + assert(!getAbstractScopeDIEs().count(DS) && + "Abstract DIE for this scope exists!"); + getAbstractScopeDIEs()[DS] = ScopeDIE; jmmartinez

[PATCH] D146466: [clang] diagnose function fallthrough

dblaikie added a comment. Yeah, we already have the return type warning and UBSan for the dynamic version of this check - seems OK? That a function lowers to unreachable isn't a bug - there's a ton of them in LLVM, quite intentionally as unimplemented functions that should never be called. If

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

dblaikie added a comment. I don't think of this as a performance regression for users though - this functionality's never really "shipped" so we get to choose what the baseline is. And I think a reasonable baseline to compare to isn't this implementation we don't think is ideal (because of the

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

dblaikie added a comment. In D144844#4193727 , @iains wrote: > In D144844#4193568 , @dblaikie > wrote: > >> Seem to recall @iains and others were concerned about the number of modules >> flags - this one I'd be

[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

dblaikie added a comment. Seem to recall @iains and others were concerned about the number of modules flags - this one I'd be inclined to suggest we shouldn't add if possible. If one way or the other is generally better - we should, at least initially, dictate that behavior entirely until

[PATCH] D145987: [ADT][NFCI] Do not use non-const lvalue-refs with enumerate in llvm/

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks alright - thanks! :) Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3962 int InitialOffset = std::get<1>(Vec[0]); - AnyConsecutive |=

[PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

dblaikie added a comment. Yeah, can't say this had occurred to me - but totally makes sense/reckon it's OK. Any reason to limit this to lldb? I'd expect it'd probably "Just Work(tm)" on any DWARF consumer? it doesn't hit any recursion issues? (I guess maybe skirts it due to the existing

[PATCH] D138247: PR58819: Correct linkage and mangling of lambdas in inline static member initializers

dblaikie added a comment. GCC bug hasn't got much traction, so I pinged it. But probably still the right thing to continue with this fix, I think. @rjmccall Could you give a quick glance/confirm my ABI understanding/diagnosis/direction here? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D145256: [clang/Diagnostic] Use `optional` to disambiguate between a `StoredDiagMessage` that is not set vs set as empty string

dblaikie added a comment. > "But when would you have a completely empty diagnostic message", you ask dear > reader? > That is when there is an empty "#warning" in code. Is this observable from a clang command line tool? Be nice to have a lit test. Repository: rG LLVM Github Monorepo

[PATCH] D145077: [clang][DebugInfo] Support DW_AT_LLVM_preferred_name attribute

dblaikie added a comment. In D145077#4168209 , @dblaikie wrote: > In D145077#4162110 , @aprantl wrote: > >> I originally was hoping we wouldn't have to introduce a new attribute for >> this, but it looks like

[PATCH] D145077: [clang][DebugInfo] Support DW_AT_LLVM_preferred_name attribute

dblaikie added a comment. In D145077#4162110 , @aprantl wrote: > I originally was hoping we wouldn't have to introduce a new attribute for > this, but it looks like there are legitimate concerns about the alternatives, > so in that sense this looks

[PATCH] D143877: [NFC] [clang] Forward forwarding reference

This revision was automatically updated to reflect the committed changes. Closed by commit rG58ec6e09abe8: [NFC] [clang] Forward forwarding reference (authored by ccotter, committed by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D144931: DebugInfo: Disable ctor homing for types with only deleted (non copy/move) ctors

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd8a1a559f300: DebugInfo: Disable ctor homing for types with only deleted (non copy/move) ctors (authored by dblaikie). Repository: rG LLVM Github

[PATCH] D144931: DebugInfo: Disable ctor homing for types with only deleted (non copy/move) ctors

dblaikie created this revision. dblaikie added a reviewer: akhuang. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Such a type is never going to have a ctor home, and may be used for type punning or

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

dblaikie added a comment. I think "mangled name" is probably the closest without being overly verbose for the average user. We do use "mangled name" in a few places already, some which look not too unrelated to this one: clang/include/clang/Basic/DiagnosticSemaKinds.td: "mangled name of %0

[PATCH] D143877: [NFC] [clang] Forward forwarding reference

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Can you commit this yourself, or would you like me to commit it on your behalf? (what name/email address would you like it attributed to?) Repository: rG LLVM

[PATCH] D143877: [NFC] [clang] Forward forwarding reference

dblaikie added a comment. In D143877#4156769 , @dblaikie wrote: >> Although we may not agree such ideas, we should offer an option for the >> users to give them the right to choose. > > I think @iains was bemoaning the large number of module flags

[PATCH] D143877: [NFC] [clang] Forward forwarding reference

dblaikie added a subscriber: iains. dblaikie added a comment. > Although we may not agree such ideas, we should offer an option for the users > to give them the right to choose. I think @iains was bemoaning the large number of module flags recently - perhaps he'd have some thoughts on this,

[PATCH] D144870: [Clang][DebugInfo] Emit zero size bitfields in the debug info to delimit bitfields in different allocation units.

dblaikie added a comment. Haven't followed the patch in detail - but why does this require changes to the RecordLayout code? (I'd expect this to only require charnges to the debug info code - maybe with a shared helper utility function that both the AMDGPU ABI implementation, and the debug

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

dblaikie added a comment. In D144181#4133056 , @Michael137 wrote: > In D144181#4133025 , @dblaikie > wrote: > >> Ah, accidentally posted to the lldb part of this stack... instead: >> >> Any chance we can make

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

dblaikie added a comment. Ah, accidentally posted to the lldb part of this stack... instead: Any chance we can make these work more like member functions (could the ctors include their mangled names, for instance)? Or is it the innate nature of ctors having the various C1

[PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

dblaikie added a comment. > I think /maybe/ we had some design principle that DWARF features wouldn't be > solely controlled by debugger tuning, the tuning only indicates defaults but > tehy can be controlled by flags? Not sure if I'm remembering that quite > right, though - maybe @probinson

[PATCH] D143877: [NFC] [clang] Forward forwarding reference

dblaikie added a comment. any chance this is testable, maybe in a unit test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143877/new/ https://reviews.llvm.org/D143877 ___ cfe-commits mailing list

[PATCH] D143806: [Clang][Test] Add llvm-lto, llvm-lto2 and llvm-profdata to the tool substitutions list

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems a bit unfortunate that clang tests are using these tools, but fair enough - they clearly are, so suitable to include them here, I guess. Repository: rG LLVM Github Monorepo

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

dblaikie added a comment. In D143803#412 , @0xdc03 wrote: > Note that as it stands currently, this patch cannot be committed because the > test `clang/test/SemaCXX/externc-ifunc-resolver.cpp` fails to run. The > contents of the test are as follows:

[PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

dblaikie added a subscriber: probinson. dblaikie added a comment. In D143501#4116200 , @Michael137 wrote: >> I'd recommend a possible long-term solution would be simplified template >> names (so we don't have to worry about encoding this in the

[PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

dblaikie added a comment. In D143501#4110302 , @Michael137 wrote: > The alternative would be attaching the preferred name as a new attribute or > an existing attribute like `DW_AT_description`. I'd recommend a possible long-term solution would be

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4100762 , @steven_wu wrote: > I don't think it is feasible with current tool to write a test that check > semantically the order of decls in clang module. (Let me know if that was > wrong). The current test already

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4100660 , @akyrtzi wrote: >> would be great to test it more in a semantic way if possible > > Keep in mind that the specific order of the decls doesn't matter for the > purposes of this test, what matters is that the

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

dblaikie added a comment. In D141451#4095303 , @nickdesaulniers wrote: > In D141451#4064298 , @dblaikie > wrote: > >> Right - I was thinking more, as above, about directly using the existing >> metadata

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4095197 , @steven_wu wrote: >> Sorry, I'm still not really following - OK, sounds like you're saying this >> test does fail at HEAD/without this patch in reverse iteration mode, and is >> a bit larger than would be

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

dblaikie added a comment. In D124351#4097110 , @cor3ntin wrote: > In D124351#4093840 , @nikic wrote: > >> FYI this causes a minor compile-time regression (around 0.35% on 7zip at >> `O0`): >>

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4094942 , @steven_wu wrote: > In D141625#4094837 , @dblaikie > wrote: > >> In D141625#4094742 , @steven_wu >> wrote: >> >>> In

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4094742 , @steven_wu wrote: > In D141625#4067225 , @dblaikie > wrote: > >> In D141625#4066961 , @steven_wu >> wrote: >> >>> No,

[PATCH] D142800: [Clang][Diagnostic] Add `-Wcomparison-op-parentheses` to warn on chained comparisons

dblaikie added a comment. > I'd be interested to see the fixit-hints for the first bit, also to see how > others feel about it here. My 2c is that `-Wparentheses` is already a very stylistic warning (even correct code, without knowing about this esoteric/specific suppression style of adding

[PATCH] D142861: [Clang] avoid relying on StringMap iteration order when roundtripping -analyzer-config

dblaikie added a comment. (heh, don't mind my feedback being duplicate - didn't refresh before submitting - glad other folks got to it before me! :) ) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142861/new/ https://reviews.llvm.org/D142861

[PATCH] D142861: [Clang] avoid relying on StringMap iteration order when roundtripping -analyzer-config

dblaikie added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:881 + // Sort options by key to avoid relying on StringMap iteration order. + SmallVector, 4> SortedConfigOpts; for (const auto : Opts.Config) { jansvoboda11 wrote: >

[PATCH] D142675: [clang][CGDebugInfo] emit DW_LANG_C_plus_plus_{20|17} DW_LANG_C17

dblaikie added a comment. This was discussed in D138597 & we decided not to implement these due to the risk of breaking existing v5 clients that might not know about the post-v5-release codes. Implementing the v6 codes as an extension might be an option, or

[PATCH] D140250: Define NULL in its own header

dblaikie added inline comments. Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll:37 !7 = !DIFile(filename: "clang/12.0.0/include/__stddef_max_align_t.h", directory: "/") -!8 = !DICompositeType(tag: DW_TAG_structure_type, file: !7, line:

[PATCH] D140250: Define NULL in its own header

dblaikie added inline comments. Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll:37 !7 = !DIFile(filename: "clang/12.0.0/include/__stddef_max_align_t.h", directory: "/") -!8 = !DICompositeType(tag: DW_TAG_structure_type, file: !7, line:

[PATCH] D141826: [WIP][clang][TemplateBase] Add IsDefaulted bit to TemplateArgument

dblaikie added a comment. In D141826#4072609 , @Michael137 wrote: > In D141826#4059088 , @erichkeane > wrote: > >> In D141826#4059073 , @Michael137 >> wrote: >> >>> In

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

dblaikie added inline comments. Comment at: clang/test/SemaCXX/class-layout.cpp:18 // expected-no-diagnostics +#if !defined(__MVS__) && !defined(_AIX) What's the reason this part is #ifdef'd out? Does it contain code that's unsupported on these platforms?

[PATCH] D142358: Opting out of Clang 15 ABI Changes for AIX and z/OS

dblaikie added inline comments. Comment at: clang/docs/ReleaseNotes.rst:822 classified such types as non-POD (for the purposes of Itanium ABI). Clang now - matches the gcc behavior (except on Darwin and PS4). You can switch back to + matches the gcc behavior (except on

[PATCH] D142268: [clang][DebugInfo] Don't canonicalize names in template argument list for alias templates

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

[PATCH] D142268: [clang][DebugInfo] Don't canonicalize names in template argument list for alias templates

dblaikie added a comment. So @Michael137 and I talked about this offline, and a few extra details: - Generally it's important that types have identical names. Templates try to do this, but get it wrong in a bunch of ways (& certainly between GCC and Clang we get it different in a bunch of ways

[PATCH] D139774: [libclang] Add API to set temporary directory location

dblaikie added a comment. In D139774#4069014 , @aaron.ballman wrote: > In D139774#4066920 , @dblaikie > wrote: > >> Don't let me hold this up - I think it all feels a bit too ad-hoc for my own >> preferences

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4066961 , @steven_wu wrote: > No, reverse iteration will not break diff test for a small number of decls. > Everything will be in reverse order so it is the same. Hmm, I'm not sure I'm following why that is - could

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4066681 , @steven_wu wrote: > In D141625#4066466 , @dblaikie > wrote: > >> In D141625#4066362 , @steven_wu >> wrote: >> >>>

[PATCH] D139774: [libclang] Add API to set temporary directory location

dblaikie added a comment. Don't let me hold this up - I think it all feels a bit too ad-hoc for my own preferences (feels like there should be fairly general solutions to this - rather than playing whack-a-mole on only the biggest temporary files - both in terms of the options KDevelop

[PATCH] D139774: [libclang] Add API to set temporary directory location

dblaikie added a comment. In D139774#4066574 , @aaron.ballman wrote: > In D139774#4066519 , @dblaikie > wrote: > >> I've mixed feelings about this, but yeah, I guess the issues with global >> state are pretty

[PATCH] D139774: [libclang] Add API to set temporary directory location

dblaikie added a comment. In D139774#4065753 , @aaron.ballman wrote: > In D139774#4063131 , @vedgy wrote: > >> After a discussion under the corresponding KDevelop merge request, I can see >> 4-6 alternative

[PATCH] D141310: [clang] add -Wcompare-function-pointers

dblaikie added a comment. >>> It's not that noisy compiling clang (eight hits). >> >> Good to know - I'm surprised it's that low. >> >> Is there some idiom we can use/document/recommend for people to use when the >> warning is a false positive? (when the user is confident the functions won't >>

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a comment. In D141625#4066362 , @steven_wu wrote: > @dblaikie Do you have any objection for committing the patch as it is? I > don't think reverse iteration test is a proper way to test for this specific > bug. I think reverse iteration

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

dblaikie added subscribers: JDevlieghere, aprantl. dblaikie added a comment. In D141451#4063582 , @nickdesaulniers wrote: > In D141451#4063519 , @dblaikie > wrote: > >> In D141451#4063504

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

dblaikie added a comment. In D141451#4063504 , @nickdesaulniers wrote: > In D141451#4063335 , @dblaikie > wrote: > >> In D141451#4063151 , >> @nickdesaulniers wrote:

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

dblaikie added a comment. In D141451#4063151 , @nickdesaulniers wrote: > In D141451#4045658 , @efriedma > wrote: > >> clang has a "LocTrackingOnly" setting for debug info, which emits DILocation >> info into

[PATCH] D141310: [clang] add -Wcompare-function-pointers

dblaikie added a comment. In D141310#4062776 , @adriandole wrote: > @dblaikie, we would use this warning in Chrome OS. Ah, good to know! > We use `icf=all` and have encountered bugs caused by function pointer > comparisons. & the savings are worth

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

dblaikie added a subscriber: colinl. dblaikie added a comment. In D141625#4059486 , @steven_wu wrote: > @dblaikie Do we have any bots running reverse iteration? Hmm, not that I can see/find at a quick glance, which is unfortunate. @mgrang Are you still

[PATCH] D141310: [clang] add -Wcompare-function-pointers

dblaikie added a comment. But I don't mean to suggest I should be a decider/veto here - it's cheap to maintain/no big deal, so maybe that's fine - but for myself, having at least some large scale customer with existing experience testing the warning and a strong commitment/motivation to keep

[PATCH] D141310: [clang] add -Wcompare-function-pointers

dblaikie added a comment. In D141310#4060069 , @aaron.ballman wrote: > In D141310#4054351 , @dblaikie > wrote: > >> @adriandole do you plan to deploy this in a codebase? Have you tried it on a >> codebase

<    1   2   3   4   5   6   7   8   9   10   >