[PATCH] D78853: [Analysis] Fix null pointer dereference warnings [1/n]

dblaikie added inline comments. Comment at: clang/lib/Analysis/ThreadSafety.cpp:1938-1940 +// The default value for the argument VD to the current function is +// nullptr. So we assert that VD is non null because we deref VD here. +assert(VD && "!VD"); ---

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

dblaikie added a comment. (peanut gallery: I'd consider, while you're touching these all anyway, changing them all to non-member (friended where required) as I believe that's best practice - allows equal implicit conversions on either side, for instance (even if some types have no implicit conv

[PATCH] D78882: [IR] Replace all uses of CallBase::getCalledValue() with getCalledOperand().

dblaikie accepted this revision. dblaikie added a comment. Looks great - thanks for the pointer type fixup(s) along the way too! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78882/new/ https://reviews.llvm.org/D78882 ___

[PATCH] D78938: Fixing all comparisons for C++20 compilation.

dblaikie added a comment. In D78938#2007571 , @BRevzin wrote: > In D78938#2007037 , @dblaikie wrote: > > > (peanut gallery: I'd consider, while you're touching these all anyway, > > changing them all to non-member

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. (updating the previous review has the advantage that reviewers can review the diff relative to the previous version of the patch, using Phabricator's "History" feature, so they can review the delta since the committed/reverted version) Repository: rC Clang CHANGES

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. Yeah, I was confused by all this too - @aganea's right - this review doesn't make sense to me, since it doesn't show a proposed change to LLVM, it shows a proposed change on top of another patch, that would necessarily be committed together/in a single commit (a propos

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/SmallVector.h:52 + // The maximum size depends on size_type used. + static constexpr size_t SizeMax() { +return std::numeric_limits::max(); smeenai wrote: > browneee wrote: > > dexonsmith wro

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dblaikie added a comment. In D77621#2013400 , @browneee wrote: > Thanks for the tips, MaskRay. > > Yes, I expect this would fix that issue. > > smeenai, SizeTypeMax() is intended to return size_t. > > --- > > I see a couple of options for fixing the trunc

[PATCH] D83788: Removed unused variable in clang

dblaikie closed this revision. dblaikie added a comment. In D83788#2183684 , @hiraditya wrote: > Not sure why this didnt get closed after the patch is merged in: > https://reviews.llvm.org/rG4fd91b0f946f49370a3ab33c480a807a3f48b247 The commit didn't cont

[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3787-3788 (void)checkDebugInfoOption(A, Args, D, TC); if (DebugInfoKind == codegenoptions::LimitedDebugInfo && NeedFullDebug) DebugInfoKind = codegenoptions::FullDebugInfo; --

[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

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/D80242/new/ https://reviews.llvm.org/D80242 _

[PATCH] D85565: [Clang][Test] add specific targets for OSX failures

dblaikie added inline comments. Comment at: clang/test/CodeGen/debug-info-unused-types.cpp:1-5 +// RUN: %clangxx --target=x86_64-linux-gnu -fno-eliminate-unused-debug-types -g -emit-llvm -S -o - %s | FileCheck %s +// RUN: %clangxx --target=x86_64-linux-gnu -fno-eliminate-unused-

[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

dblaikie added inline comments. Comment at: clang/test/Driver/debug-options.c:371-373 +// RUN: %clang -### -fno-eliminate-unused-debug-types -g1 -c %s 2>&1 \ +// RUN:| FileCheck -check-prefix=NO_DEBUG_UNUSED_TYPES %s +// NO_DEBUG_UNUSED_TYPES: "-debug-info-kind={{limited

[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

dblaikie accepted this revision. dblaikie added a comment. Looks good, thanks! Comment at: clang/test/Driver/debug-options.c:371-373 +// RUN: %clang -### -fno-eliminate-unused-debug-types -g1 -c %s 2>&1 \ +// RUN:| FileCheck -check-prefix=NO_DEBUG_UNUSED_TYPES %s +// NO

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. This seems like a strange hybrid between a static-polymorphism (with traits) and dynamic polymorphism (with base classes/virtual functions). Could this more readily be just one or the other? (sounds like you're leaning towards dynamic polymorphism) Repository: rG L

[PATCH] D85799: [DebugInfo] Add -fuse-ctor-homing cc1 flag so we can turn on constructor homing only if limited debug info is already on.

dblaikie added a comment. Looks good! Could you add a test case for this too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85799/new/ https://reviews.llvm.org/D85799 ___ cfe-commits mailing list cfe-com

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2213797 , @nhaehnle wrote: > In D83088#2208611 , @dblaikie wrote: > >> This seems like a strange hybrid between a static-polymorphism (with traits) >> and dynamic polymorphism (w

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2213864 , @nhaehnle wrote: > In D83088#2213802 , @dblaikie wrote: > >> In D83088#2213797 , @nhaehnle wrote: >> >>> In D83088#2208611

[PATCH] D85799: [DebugInfo] Add -fuse-ctor-homing cc1 flag so we can turn on constructor homing only if limited debug info is already on.

dblaikie added a comment. If possible, could you test the negative cases too? That -fuse-ctor-homing doesn't override -debug-info-kind=line-tables-only or no -debug-info-kind at all? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85799/new/ https:

[PATCH] D85799: [DebugInfo] Add -fuse-ctor-homing cc1 flag so we can turn on constructor homing only if limited debug info is already on.

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

[PATCH] D85474: Add -fbinutils-version= to gate ELF features on the specified binutils version

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4642 +int Num; +if (V == "future") + A->render(Args, CmdArgs); ro wrote: > MaskRay wrote: > > phosek wrote: > > > Another option would be `unstable`. > > I picked `futu

[PATCH] D83088: Introduce CfgTraits abstraction

dblaikie added a comment. In D83088#2218559 , @nhaehnle wrote: > In D83088#2213886 , @dblaikie wrote: > >> In D83088#2213864 , @nhaehnle wrote: >> >>> In D83088#2213802

[PATCH] D76646: Rename/refactor isIntegerConstantExpression to getIntegerConstantExpression

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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie requested changes to this revision. dblaikie added a comment. This revision now requires changes to proceed. So the original commit ( cbc9d22e49b4 ) was reverted at some point, and now you're proposing recommitting it

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

dblaikie added a comment. In D69778#2035006 , @llunak wrote: > In D69778#2032363 , @dblaikie wrote: > > > So the original commit ( cbc9d22e49b4 > >

[PATCH] D79967: Fix debug info for NoDebug attr

dblaikie added a comment. I'm not quite following the test case - could you explain how it's testing the situation? It might be better/easier to have a more intentional test - also, could you check the commit history for this declaration-emitting functionality (CGDebugInfo::EmitFunctionDecl wh

[PATCH] D80001: [RFC/WIP][clang] Fix printing of names of inherited constructors

dblaikie added a comment. @rsmith - this changes printing the name of inherited constructors in general, not just for debug info - could you take a gander and make sure that's probably OK for other callers trying to print the name of an inheriting constructor? Repository: rG LLVM Github Mono

[PATCH] D79967: Fix debug info for NoDebug attr

dblaikie added a subscriber: vsk. dblaikie added a comment. Could you check the commit history for this feature and rope in some folks who added the function declaration work (it's for debug call sites) - maybe @vsk is the right person, or knows who is, to check this is the right fix for it/doe

[PATCH] D79967: Fix debug info for NoDebug attr

dblaikie added subscribers: djtodoro, dexonsmith. dblaikie added a comment. In D79967#2041595 , @yaxunl wrote: > In D79967#2039153 , @dblaikie wrote: > > > Could you check the commit history for this feature and rop

[PATCH] D74572: [BPF] preserve debuginfo types for builtin __builtin__btf_type_id()

dblaikie added subscribers: JDevlieghere, probinson, aprantl, echristo, dblaikie. dblaikie added a comment. This seems like a fairly invasive way to preserve certain DWARF types. Are you in a position to make source changes to help here? Would it be reasonable to annotate these types with __att

[PATCH] D74572: [BPF] preserve debuginfo types for builtin __builtin__btf_type_id()

dblaikie added a comment. I think it'd probably be good to revert this/other patches, and maybe have a design discussion on cfe-dev. Could you do that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74572/new/ https://reviews.llvm.org/D74572 ___

[PATCH] D74668: [Clang][BPF] implement __builtin_btf_type_id() builtin function

dblaikie added a comment. (mentioned also on the LLVM side of this D74572 ) If this is in the interests of retaining certain types in the emitted debug info - it seems quite complicated compared to what I'd hope for. Would it be sufficient to support __attribut

[PATCH] D40601: [XRay][clang] Introduce -fxray-always-emit-customevents

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Not totally clear to me why this feature is desirable, but assume you've got use cases/reasons :) Comment at: clang/lib/Driver/XRayArgs.cpp:32-33 constexpr char XRayNev

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

dblaikie added a comment. (generally seems OK - maybe include a comment about this being a less-than-perfect solution (the better solution would be to encode this in IR metadata (DICompileUnit) if it's reasonable to respect this option on a per-CU basis (which it probably is, though that'd be a

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

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

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

dblaikie added a comment. any chance of generalizing this beyond a special case for `std::vector` - an attribute we could add to any class template (perhaps particular template parameters to the class template) to document the requirement? (is std::vector the only type in the standard library t

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

dblaikie added subscribers: hansw, dblaikie. dblaikie added a comment. Got any pointers to the commits/authors that added this functionality in the first place? (would be good to have any review history, etc, to check against the change here) @hansw for windows compatibility discussions Repos

[PATCH] D132909: [msan] Use Debug Info to point to affected fields

dblaikie added a comment. The code change looks like it's creating `inlinedAt` scopes, but none of the test cases seem to test any `inlinedAt` debug info? Looks like this is lacking some test coverage - could you add some? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D132874: [clang] Don't emit debug vtable information for consteval functions

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1763-1766 if (Method->isPure()) SPFlags |= llvm::DISubprogram::SPFlagPureVirtual; else SPFlags |= llvm::DISubprogram::SPFlagVirtual; Seems this change means the

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

dblaikie accepted this revision. dblaikie added a comment. In D133425#3777615 , @aaron.ballman wrote: > In D133425#3777598 , @aaron.ballman > wrote: > >> In D133425#3775353

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

dblaikie added a comment. > In D133425#3778433 , @dblaikie > wrote: > >> So previously/currently-without-this-patch the diagnostic was suppressed if >> the use of ctad was in a system header (suppression based on the >> generic/builtin diagnostic suppre

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

dblaikie added a comment. In D133354#3778281 , @ningvin wrote: > In D133354#3778237 , @dblaikie > wrote: > >> Got any pointers to the commits/authors that added this functionality in the >> first place? (would b

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

dblaikie added a comment. In D133354#3779882 , @ningvin wrote: > In D133354#3779123 , @dblaikie > wrote: > >> That works, you can also take the hash with the `rG` prefix and use that: >> rGd577fbbd1c9d6dab193d53

[PATCH] D133425: Silence -Wctad-maybe-unsupported stemming from system headers

dblaikie added a comment. In D133425#3780435 , @ldionne wrote: > In D133425#3779121 , @dblaikie > wrote: > >>> One thing I don't understand in the current state of things is why the >>> diagnostic fires at all i

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

dblaikie added a comment. In D133092#3787247 , @azat wrote: > Is there anything left here? Generally not. the usual flow would be once you have approval to go ahead and submit the patch yourself - I take it you don't have commit access? In which case I

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

This revision was automatically updated to reflect the committed changes. Closed by commit rG6bf6730ac55e: [clang] fix generation of .debug_aranges with LTO (authored by azat, committed by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1330

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

dblaikie added a comment. In D133092#3787379 , @azat wrote: >> the usual flow would be once you have approval to go ahead and submit the >> patch yourself - I take it you don't have commit access? > > Yeah, I don't > I though that only trusted/experience

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

dblaikie created this revision. dblaikie added reviewers: ayzhao, hansw. Herald added subscribers: mstorsjo, kristof.beyls. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Details posted here: https://r

[PATCH] D133847: [test][clang] fix pattern for LDGARANGE-NOT in debug-options-lld test

dblaikie added a comment. Presumably this would break if someone's build uses a tool suffix (so lld becomes lld-tot or something like that)? Perhaps `lld[^"]*"` would be better? Are there other test cases that test something similar that we could look at to see how they work/more likely get some

[PATCH] D133875: [clang] fix generation of .debug_aranges with LTO (resubmit)

dblaikie added a comment. I think @maskray was saying this doesn't actually work & different flags are needed to pass to lld - have you tested this end-to-end/with a real compilation, does that work? (are there other cases that already use `-mllvm` to pass flags down to lld?) Repository: rG

[PATCH] D133875: [clang] fix generation of .debug_aranges with LTO (resubmit)

dblaikie added inline comments. Comment at: clang/test/Driver/debug-options-aranges.c:15 +// RUN: %clang -g -target x86_64-linux -flto=thin -fuse-ld=lld -gdwarf-aranges %s -o - | llvm-dwarfdump --debug-aranges - | FileCheck -check-prefix=ARANGES %s +// ARANGES: Address Range Hea

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Fair enough - all seems a bit unfortunate to be pushing these attributes through to places they don't technically apply to (both complicates the implementation, and might be confusing to u

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

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

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

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

[PATCH] D128667: [WIP] Add Zstd ELF support

dblaikie added a comment. Yeah, probably worth splitting this up a bit - somewhat hard to split up generation+parsing in LLVM itself (eg: llvm-mc+llvm-objdump), so maybe they're grouped together (alternatively the objdump support gets checked in first with precompiled/binary test files - I forg

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

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

[PATCH] D119409: [C++20] [Modules] Remain dynamic initializing internal-linkage variables in module interface unit

dblaikie added a comment. is this an issue for Clang Header Modules codegen too? (maybe the solution should be the same for both, then) & maybe we should do this regardless of the presence/absence/type of initializer, just for consistency? CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D121328: Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

dblaikie updated this revision to Diff 417985. dblaikie added a comment. Try Dean's suggestion of moving this check further down Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121328/new/ https://reviews.llvm.org/D121328 Files: clang/lib/Sema/Sem

[PATCH] D121328: Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

dblaikie updated this revision to Diff 417989. dblaikie added a comment. Add regression test for the -Wnon-c-typedef-for-linkage issue Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121328/new/ https://reviews.llvm.org/D121328 Files: clang/lib/Se

[PATCH] D121328: Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

dblaikie added a comment. In D121328#3405623 , @deansturtevant wrote: > Aha! (I think). > If the code to test "isExternalVisible" is executed *after* the code to test > whether it's a C++ member function (the very next test), then the problem > wouldn'

[PATCH] D121328: Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

dblaikie added a comment. @aaron.ballman wouldn't mind your take on this to see if this seems sufficiently robust, tested, etc. (should I move the isExternallyVisible check even further down? So its side effects are even less impactful (maybe there are other warnings that care about this sort o

[PATCH] D121328: Disable -Wmissing-prototypes for internal linkage functions that aren't explicitly marked "static"""

This revision was automatically updated to reflect the committed changes. Closed by commit rG34b9b1ea4874: Disable -Wmissing-prototypes for internal linkage functions that aren't… (authored by dblaikie). Changed prior to commit: https://reviews.llvm.org/D121328?vs=417989&id=418360#toc Reposito

[PATCH] D130566: [Driver] Default to DWARF 4 on Linux/sparc64

dblaikie added a comment. > During a build on Debian 11/sparc64, several binaries fail to link with > > /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23 What's the linker doing parsing DWARF? Any idea what feature this is? And why is it SPARC-specific? Or is it true

[PATCH] D130055: Clang extensions yolo, woot & kaboom

dblaikie added a comment. In D130055#3682497 , @beanz wrote: > In D130055#3677743 , @dblaikie > wrote: > >> Any chance this could be a generalization of >> https://clang.llvm.org/doxygen/Consumed_8cpp_source.htm

[PATCH] D130566: [Driver] Default to DWARF 4 on Linux/sparc64

dblaikie added a comment. > This is all extremely weird, but until the error shows up again, I'll put > this patch on hold (not yet abandoning since there seems to be no way to > unabandon if necessary). You can abandon and then "reclaim" the revision to reopen it and continue work. Repositor

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

dblaikie added a comment. In D123319#3681884 , @Michael137 wrote: > In D123319#3680169 , @aprantl wrote: > >> Sorry for the silence — I was out on vacation. >> >> @Michael137 has recently started looking into imp

[PATCH] D130566: [Driver] Default to DWARF 4 on Linux/sparc64

dblaikie requested changes to this revision. dblaikie added a comment. This revision now requires changes to proceed. I'll flag this as "request changes" so we're clear the issue isn't fully understood nor fully reproducible - so once there's some more data on the scope/impact/cause/timeline for

[PATCH] D130516: compression classes

dblaikie added a comment. Any chance this could be split up to be more directly comparable to https://reviews.llvm.org/D130458 ? Comment at: clang-tools-extra/clangd/index/Serialization.cpp:32 +llvm::compression::CompressionAlgorithm *StringTableCompressionScheme = +new ll

[PATCH] D130516: compression classes

dblaikie added inline comments. Comment at: clang-tools-extra/clangd/index/Serialization.cpp:32 +llvm::compression::CompressionAlgorithm *StringTableCompressionScheme = +new llvm::compression::ZlibCompressionAlgorithm(); + ckissane wrote: > dblaikie wrote: >

[PATCH] D130516: [llvm] compression classes

dblaikie added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:50-58 + compression::CompressionAlgorithm *CompressionScheme = + new compression::ZlibCompressionAlgorithm(); + + CompressionScheme = + CompressionScheme->when(Compress

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3688123 , @MaskRay wrote: > I'd like to make a few arguments for the current namespace+free function > design, as opposed to the class+member function design as explored in this > patch (but thanks for the exploratio

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

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

[PATCH] D130791: [clang] Short-circuit trivial constexpr array constructors

dblaikie added a comment. > Thank you for working on this! This should also include test coverage (hmmm, > we don't have a reasonable way to lit test performance regressions though > so perhaps we just need to ensure there's at least one test that would > normally have been unreasonably slo

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. (still lots of outstanding comments from my last round, so I won't reiterate those - but waiting for some responses to them) Comment at: llvm/lib/Support/Compression.cpp:30-32 +ZStdCompressionAlgorithm +*llvm::compression::ZStdCompressionAlgorithm

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3694151 , @MaskRay wrote: > In D130516#3688236 , @dblaikie > wrote: > >> In D130516#3688123 , @MaskRay >> wrote: >> >>> I'd like to

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. The current code here still seems more complicated than I'd prefer - looks like currently the size/speed/default levels are currently unused, so maybe we can omit those for now, knowing they will be added? And the CompressionKind with all its operator overloads seems li

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. > I think I have worked out something that is the best of both worlds: I think @MaskRay's main concern, which I share to a degree, is that there's a lot of code/complexity here that doesn't currently seem warranted by the size of the problem. So adding more implementat

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. > Yes, I can continue to trim down the implementation! I agree with your > sentiment. Thanks! This update helps - though I think we'll still want to further isolate the different pieces of this change/reduce this further. > I agree with some of this, I have some stron

[PATCH] D130516: [llvm] compression classes

dblaikie added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

dblaikie added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. This is looking pretty close to what I've been picturing - the only thing remaining is that I think we could get rid of `CompressionKind` and access the `CompressionAlgorithm` directly - basically the contents of `CompressionKind::operator->` could be a free/public fun

[PATCH] D131368: [Basic] Deprecate MapEntryOptionalStorage::{hasValue,getValue}

dblaikie added a comment. Update this to use something like https://reviews.llvm.org/D131381 ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131368/new/ https://reviews.llvm.org/D131368 ___ cfe-commits m

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

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

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

dblaikie added a comment. Any update on this? (& it seems like maybe the "why use auto at all" question got lost? Might be worth engaging in that question - if we decide it's not worth using then maybe we don't need any lldb fixes and can remove the functionality from clang entirely? (though I

[PATCH] D131368: [Basic] Deprecate MapEntryOptionalStorage::{hasValue,getValue}

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good - I'd be probably OK with removing these outright as they're so niche/generally only for use by `llvm::Optional` anyway - but not much harm in keeping them around for the same

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added a subscriber: aaron.ballman. dblaikie added a comment. @aaron.ballman is this something you're comfortable reviewing or could recommend anyone else who might be suitable? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ htt

[PATCH] D131388: [docs] Add "C++20 Modules"

dblaikie added inline comments. Comment at: clang/docs/CPlusPlus20Modules.rst:31 + +This document was intended to be a manual first and foremost, however, we consider it helpful to +introduce some language background here for readers who are not familiar with

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3710903 , @ckissane wrote: > In D130516#3703691 , @dblaikie > wrote: > >> This is looking pretty close to what I've been picturing - the only thing >> remaining is that I thi

[PATCH] D131388: [docs] Add "C++20 Modules"

dblaikie added a comment. (I'll +1 to @ruoso's comments about using BMI - I think it might be best not to introduce a different term ("Module file") as it might confuse people as it's fairly close to "module source file", etc. BMI makes it clear/unambiguous that it's the binary file, not the so

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie updated this revision to Diff 451686. dblaikie added a comment. Remove driver flag (just rely on ABI compat) & tidy up testing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/inclu

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

dblaikie added a comment. Still waiting for the pre-merge checks to complete, but hopefully this is clean now. Realized maybe we don't need a separate driver flag for this at all, and rely only on the abi-compat flag? That seems to be how (at least some) other ABI compat changes have been hand

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3710972 , @MaskRay wrote: > I have only taken very brief look at the new version. Having an enum class > `CompressionKind` with a parallel `CompressionAlgorithm` seems redundant. > `friend CompressionAlgorithm *Compre

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

dblaikie added a comment. Direction seems pretty good - some of the details I'm fuzzy on. If there's easy ways to keep some of the existing syntax (like I see at least one loop that I think was querying the incremented iterator for "isAtEnd" - maybe that can be preserved initially) to reduce th

[PATCH] D131464: [test] Make tests pass regardless of gnu++14/gnu++17 default

dblaikie added a comment. > I use something like > > // RUN: %clang_cc1 %s -fsyntax-only -verify=expected,precxx17 -std=c++14 > -fdata-sections -fcolor-diagnostics > // RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++17 -fdata-sections > -fcolor-diagnostics > > ... > TypedefAlign

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie accepted this revision. dblaikie added subscribers: probinson, aprantl. dblaikie added a comment. This revision is now accepted and ready to land. Please wait for sign-off from @aprantl (or another appropriate Apple representative) & @probinson (or another appropriate Sony representative

[PATCH] D104619: [clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

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

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie added a comment. In D106084#2886659 , @jmorse wrote: > This is going to be excellent for linux targets and similar, > > In D106084#2882970 , @probinson > wrote: > >> + @jmorse who is better placed than I

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie added a comment. In D106084#2890469 , @probinson wrote: >> (hence the renaming of "limited" a long time ago to "standalone-debug" to >> create a policy/philosophy around what goes in each category). > > Sorry, what? I thought -fstandalone-debug

[PATCH] D101566: Let -Wweak-template-vtables warn on implicit instantiations

dblaikie added a comment. In D101566#2891699 , @aaronpuchert wrote: > @dblaikie, does https://bugs.llvm.org/show_bug.cgi?id=18733#c17 or the > following comment change anything about your position? No, not really. This patch is still conflating two th

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