[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

dblaikie added a comment. In D80369#2051681 , @aprantl wrote: > In D80369#2051278 , @dblaikie wrote: > > > @aprantl can you check here? I've attached two IR files for the > > ModuleDebugInfo.m test, before/after ('

[PATCH] D80554: [DebugInfo] Use SplitTemplateClosers (foo >) in DWARF too

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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

dblaikie added a comment. In D80369#2056094 , @aprantl wrote: > > How does this data get used for Swift code and ObjC interoperability? At > > the moment I see no use of this IR metadata in LLVM. Does ObjC/Swift > > interop use the DI IR over in the Swif

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

dblaikie added a comment. In D80369#2057077 , @djtodoro wrote: > >> @dblaikie wrote: > > > > ... At least for the C++ test, this change makes it pass: > > > > diff --git clang/test/Modules/ModuleDebugInfo.cpp > > clang/test/Modules/ModuleDebugInfo.cpp

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

dblaikie added a comment. In D80369#2057874 , @aprantl wrote: > > The declaration subprograms are in the retained types list - but when > > retained types are emitted, only types in the retained types list are used, > > the subprograms (and the types the

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

dblaikie added inline comments. Comment at: clang/test/Modules/ModuleDebugInfo.m:46-47 -// The forward declaration should not be in the module scope. -// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file - Did this type go missing wit

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

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5369 if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never) - DebugInfo->completeUnusedClass(cast(*D)); + DebugInfo->completeUnusedClass(*CRD); } ---

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

dblaikie added inline comments. Comment at: clang/test/Modules/ModuleDebugInfo.m:46-47 -// The forward declaration should not be in the module scope. -// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file - djtodoro wrote: > dblaikie w

[PATCH] D80836: Support GCC [[gnu::attributes]] in C2x mode

dblaikie added a comment. (sorry @erichkeane didn't mean to usurp your feedback by approving before it was answered - @aaron.ballman do treat my approval as contingent on that feedback being addressed as you/both see fit) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80836/new/ https:

[PATCH] D80836: Support GCC [[gnu::attributes]] in C2x mode

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good to me (welcome to wait for other reviews if you're looking for something more nuanced/exuperienced in this area) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80836/ne

[PATCH] D80840: [Clang][CGM] style cleanups NFC

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

[PATCH] D62230: [CGDebugInfo] return early on failed dyn_cast

dblaikie added a comment. Looks like this may've already been addressed in cfee2efc57b27ce7eed932528e219a99f934d3ca ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62230/new/ https://reviews.llvm.org/D62230 _

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

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5559 + case Decl::Typedef: +if (getCodeGenOpts().DebugUnusedTypes) + if (CGDebugInfo *DI = getModuleDebugInfo()) nickdesaulniers wrote: > dblaikie wrote: > > Probably test t

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

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5565 +if (CGDebugInfo *DI = getModuleDebugInfo()) + if (getCodeGenOpts().DebugUnusedTypes) { +QualType QT = getContext().getTypedefType(cast(D)); nickdesaulniers wrot

[PATCH] D80883: [Driver] Add multiclass OptInFlag and OptOutFlag to simplify boolean option definition

dblaikie added a comment. Seems like a reasonable direction - presumably every flag (that has a similar form in both driver and frontend) would be classified as one of these (opt in or opt out)? The ones you've migrated are just a sample/example? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D80883: [Driver] Add multiclass OptInFlag and OptOutFlag to simplify boolean option definition

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Oh, meant to approve on the last. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80883/new/ https://reviews.llvm.org/D80883 __

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

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5385 + if (getCodeGenOpts().hasMaybeUnusedDebugInfo() && CRD->hasDefinition()) +DI->completeUnusedClass(*CRD); + else if (auto *ES = D->getASTContext().getExternalSource()) -

[PATCH] D83652: Merge some of the PCH object support with modular codegen

dblaikie added a comment. @hans - could you perhaps give me a quick summary of commands I could use to test this feature in Chromium (or anything else you might suggest) on a Linux box? I don't have a Windows machine, or any projects that use PCH. (or if you'd be willing to test this, that'd be

[PATCH] D83652: Merge some of the PCH object support with modular codegen

dblaikie added a comment. In D83652#2161924 , @hans wrote: > In D83652#2159585 , @dblaikie wrote: > > > @hans - could you perhaps give me a quick summary of commands I could use > > to test this feature in Chromium

[PATCH] D83652: Merge some of the PCH object support with modular codegen

dblaikie added a comment. @hans @llunak - sounds like you're both fine with this? Either of you mind to give it a formal approval, if that's the case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83652/new/ https://reviews.llvm.org/D83652 ___

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

dblaikie added a comment. In D52296#2165370 , @Trass3r wrote: > In D52296#1285328 , @grimar wrote: > > > In D52296#1284130 , @probinson > > wrote: > > > > > Only DWARF suppo

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

dblaikie added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3842-3846 + if (EmitDwarf && + Args.hasFlag(options::OPT_fno_eliminate_unused_debug_types, + options::OPT_feliminate_unused_debug_types, false) && + DebugInfoKind >= cod

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks OK (for future reference, this sort of stuff should've been cleaned up before enabling the flag so as to avoid this kind of hurry/breakage/etc) Repository: rG LLVM Github Monorepo

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

dblaikie added a comment. In D84244#2167661 , @logan-5 wrote: > In D84244#2167625 , @hans wrote: > > > I don't really know why this doesn't happen with other warning flags, but I > > think it would be better to add

[PATCH] D83652: Merge some of the PCH object support with modular codegen

This revision was automatically updated to reflect the committed changes. Closed by commit rGb198de67e0ba: Merge some of the PCH object support with modular codegen (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83652/new/ htt

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

dblaikie added a comment. In D77341#2171305 , @asbirlea wrote: > The increase in number of instructions and cycles was caused by reversing the > order in which the updates are applied by GraphDiff. > I'll look into re-adding some of the original cleanups

[PATCH] D81865: [clang] Use string tables for static diagnostic descriptions

dblaikie added a comment. In D81865#2176148 , @froydnj wrote: > In D81865#2176014 , @bkramer wrote: > > > Nice, those relocations have annoyed me for years. I'm worried about > > whether the way you're accessing St

[PATCH] D84713: [DominatorTree] Simplify ChildrenGetter.

dblaikie added a comment. In D84713#2177408 , @kuhar wrote: > LGTM. > > One tiny nit: the function name `ChildrenGet` sounds kind of backwards to me, > but it seems like the other direction is already taken. If there are both "ChildrenGet" and "GetChild

[PATCH] D81865: [clang] Use string tables for static diagnostic descriptions

dblaikie added a comment. In D81865#2178542 , @froydnj wrote: > In D81865#2176589 , @dblaikie wrote: > >> I believe this falls under the (using cppreference ( >> https://en.cppreference.com/w/cpp/language/union ) ,

[PATCH] D79147: Switch to using -debug-info-kind=constructor as default (from =limited)

dblaikie added a subscriber: echristo. dblaikie added a comment. In D79147#2178104 , @russell.gallop wrote: > Hi, > > It looks like is causing one of the debuginfo-tests: > llgdb-tests/nrvo-string.cpp to fail, run on Linux. Failure as below. I don't > t

[PATCH] D80391: [Driver] Don't make -gsplit-dwarf imply -g2

dblaikie added a comment. In D80391#2179182 , @MaskRay wrote: > Ping... Was hoping to get some more discussion on the general debug flag naming thread on the GCC list, but that hasn't happened - so I've replied/poked it a bit: https://gcc.gnu.org/piperm

[PATCH] D84870: [DebugInfo] Fix to ctor homing to ignore classes with trivial ctors.

dblaikie added a comment. I think it'd be good to separate these two issues/patches. In part because I'm curious whether, even with a trivial ctor - if we did the retained types thing - would that be enough? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D84870: [DebugInfo] Fix to ctor homing to ignore classes with trivial ctors.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D84870#2183096 , @akhuang wrote: > remove change to add class types to retained types list, > > also fix the test case to be a struct that previo

[PATCH] D77432: [DebugInfo] Change to constructor homing debug info mode: skip literal types

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

[PATCH] D77432: [DebugInfo] Change to constructor homing debug info mode: skip literal types

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2263 if (DebugKind == codegenoptions::DebugInfoConstructor && - !CXXDecl->isLambda() && !isClassOrMethodDLLImport(CXXDecl)) { -for (const auto *Ctor : CXXDecl->ctors()) { + !CXXDecl->is

[PATCH] D76646: Rename/refactor isIntegerConstantExpression to getIntegerConstantExpression

dblaikie added a comment. 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@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

dblaikie added inline comments. Comment at: llvm/include/llvm/IR/CFGDiff.h:198 +namespace { +template kuhar wrote: > kuhar wrote: > > What benefit does an anonymous namespace in a header have over a named one, > > e.g., `detail`/`impl`? Doesn't it make it mor

[PATCH] D77611: [Sema] fix -Wunused-result in StmtExpr

dblaikie added a comment. (side note: Clang doesn't strive to emulate GCC's warnings bug-for-bug/exactly - certainly about tradeoffs in terms of true and false positives to decide what's appropriate for a clang warning) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D77611: [Sema] fix -Wunused-result in StmtExpr

dblaikie added a comment. In D77611#1965824 , @nickdesaulniers wrote: > ah, using this to build a Linux kernel triggers tons of warnings. So the > behavior of GCC is even more subtle than captured in the test cases here. At least one thing I noticed -

[PATCH] D77611: [Sema] fix -Wunused-result in StmtExpr

dblaikie added a comment. In D77611#1967488 , @nickdesaulniers wrote: > Note your test case is related to `-Wunused-value`, mine is `-Wunused-result` > (via the use of `__attribute__((warn_result_unused))` which we should always > warn about. Right, b

[PATCH] D77611: [Sema] Check calls to __attribute__((warn_unused_result)) from StmtExprs

dblaikie added a comment. Looks like GCC warns on everything but the last unused expression (the last one being the return statement) - only warning on that last one if it's ultimately unused by the statement expression function call, as it were (& is annotated warn_unused_result). Basically, i

[PATCH] D68049: Propeller: Clang options for basic block sections

dblaikie added a comment. In D68049#1971276 , @MaskRay wrote: > In D68049#1970825 , @tmsriram wrote: > > > Ping. > > > @rsmith ^^^ > > More specific question, do you think > `clang/test/CodeGen/basicblock-sections.

[PATCH] D77701: [Sema] refactor static functions into private methods NFC

dblaikie added a comment. Yeah, if these were already member-function static, I'd agree it was a code smell - but function-scope static functions that take "this" as the first parameter don't seem like a bad thing to me - it reduces the header surface area/dependence. Repository: rG LLVM Gi

[PATCH] D76269: [opaque pointer types] Remove deprecated Instruction/IRBuilder APIs.

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

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dblaikie added a comment. In D77621#1970015 , @dexonsmith wrote: > In D77621#1968647 , @browneee wrote: > > > Do we want to increase the complexity of SmallVector somewhat? or do we > > want to keep the limit and a

[PATCH] D77611: [Sema] Check calls to __attribute__((warn_unused_result)) from StmtExprs

dblaikie added a comment. In D77611#1968236 , @nickdesaulniers wrote: > In D77611#1968227 , @dblaikie wrote: > > > Looks like GCC warns on everything but the last unused expression (the last > > one being the retu

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

dblaikie added a comment. In D77683#1973757 , @jdoerfert wrote: > In D77683#1970826 , @mehdi_amini > wrote: > > > I am still not sure what "if someone has asked for extra review of a > > specific area" refers to?

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

dblaikie added inline comments. Comment at: llvm/include/llvm/Support/GenericDomTree.h:32 #include "llvm/ADT/SmallVector.h" +#include "llvm/IR/CFGDiff.h" #include "llvm/Support/CFGUpdate.h" mehdi_amini wrote: > This looks like a layering violation to me here: I

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

dblaikie added a comment. Please update the patch description/subject line. @dexonsmith I'll leave this to you for final approval, since it was your idea/you've been touching things here. But looks like about the right direction. Comment at: llvm/include/llvm/ADT/SmallVector.

[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:53 + // The maximum size depends on size_type used. + size_t SizeMax() { return size_type(-1ULL); } dblaikie wrote: > I'd probably use numeric_limits here & make this static const

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

dblaikie added a comment. In D77683#1978070 , @hfinkel wrote: > In D77683#1973762 , @dblaikie wrote: > > > In D77683#1973757 , @jdoerfert > > wrote: > > > > > In D77683#1970

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

dblaikie added a comment. Looks good to me at this point (I have some vague quandries about whether the report_fatal_error stuff could be improved/made more clear, but couldn't come up with an actionable suggestion so far) - @dexonsmith could you check this over and offer final approval? Repo

[PATCH] D76646: Rename/refactor isIntegerConstantExpression to getIntegerConstantExpression

dblaikie added a comment. 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@lists.llvm.org https://lists.llvm.org/cg

[PATCH] D81131: [DebugInfo] Fix assertion for extern void type

dblaikie added a comment. Patches without tests shouldn't be approved - and at least the usual/rough metric I use for patch approval is (most often - unless I'm reviewing the work of the code owner in any area who wants a second opinion, etc) - I approve it if I'd be willing to commit a similar

[PATCH] D81131: [DebugInfo] Fix assertion for extern void type

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

[PATCH] D81522: Fix variables used only in asserts.

dblaikie added a comment. Thanks for the fix! Totally within the realm of post-commit review so far as I can see. The paired assertions could maybe be rewritten as: "assert(MRMgr.getVarRegion(P, SFC) ->getKind()== (SFC->inTopFrame() ? NonParamVarRegionKind : ParamVarRegionKind));" though it's n

[PATCH] D81732: [clang] Replace Decl::isUnconditionallyVisible() with Sema::isVisible()

dblaikie added a comment. Test case(s)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81732/new/ https://reviews.llvm.org/D81732 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

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

dblaikie added a comment. > - fix docs (use declare consistently, avoid define) Defined would be the right word here (declaring a type is code like "struct foo;" - and this change probably doesn't/shouldn't emit debug info for type declarations, yes?) Comment at: clang/inclu

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

dblaikie added a comment. In D74572#2042931 , @yonghong-song wrote: > @dblaikie I can revert but let me first understand the alternative way to do > the work, at least in high level. > > [ > I do believe my commit message, esp. the first couple of lines

[PATCH] D81732: [clang] Replace Decl::isUnconditionallyVisible() with Sema::isVisible()

dblaikie added a comment. In D81732#2094735 , @mboehme wrote: > In D81732#2093260 , @dblaikie wrote: > > > Test case(s)? > > > Is there anything specific you have in mind? > > This change should be behavior-preservi

[PATCH] D83084: DomTree: Remove the releaseMemory() method

dblaikie added a comment. @arsenm - if you can, please include some text whenever approving a patch via phabricator, otherwise no email indicating approval is sent to the mailing lists (which makes auditing reviews difficult) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. Generally Clang tries to avoid stylistic warnings like this (& they are left to be addressed by tools like clang-tidy), hence why -Winconsistent-missing-override was implemented that way (the presence of at least one "override" being a signal the user intended to use o

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2133951 , @Quuxplusone wrote: > In D82728#2133720 , @dblaikie wrote: > > > (the presence of at least one "override" being a signal the user intended > > to use override and misse

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

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:103-118 + case Decl::CXXRecord: // struct/union/class X; [C++] +if (CGDebugInfo *DI = getDebugInfo()) + if (CGM.getCodeGenOpts().hasMaybeUnusedDebugInfo() && + cast(D).hasDefinition()) +

[PATCH] D82617: Disable GCC's -Woverloaded-virtual, which has false positives.

dblaikie added a comment. In D82617#2119394 , @sammccall wrote: > Abandoning, we'll do this in clangd or find an acceptable way to silence it > (see D82736 ). > > In D82617#2119144

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2135142 , @logan-5 wrote: > Glad this is generating some discussion. For my $0.02, I would also > (obviously) love to be able to enable this warning on all the codebases I > work on, and this patch was born out of a di

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2136887 , @logan-5 wrote: > In D82728#2135149 , @dblaikie wrote: > > > Is the implementation you're proposing fairly consistent with GCC's? Run it > > over any big codebases to c

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. I'm generally on board with this, but would like @rsmith 's sign off to be sure. I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't f

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. In D82728#2137061 , @logan-5 wrote: > In D82728#2137021 , @dblaikie wrote: > > > I think it might be nice to make the -Wno-inconsistent-missing-override > > -Wsuggest-override situation a

[PATCH] D79147: Switch to using -debug-info-kind=constructor as default (from =limited)

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Could you include some comparative data in the description/commit message demonstrating this generally ends up emitting all the same types for a clang build before/after (& explanations fo

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie added a comment. Generally looks good to me (description/commit message should be updated now that the inconsistent inconsistency is no longer an issue) Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:1 +// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -

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

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69778/new/ https://reviews.llvm.org/D69778 ___ cfe

[PATCH] D82728: [clang] Add -Wsuggest-override

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D82728#2142071 , @logan-5 wrote: > Feels like a dumb question, but I'm not sure if and how those build failures > are related to this patch? The

[PATCH] D82728: [clang] Add -Wsuggest-override

This revision was automatically updated to reflect the committed changes. Closed by commit rG67895d47: [clang] Add -Wsuggest-override (authored by logan-5, committed by dblaikie). Changed prior to commit: https://reviews.llvm.org/D82728?vs=276248&id=277308#toc Repository: rG LLVM Github

[PATCH] D83648: [Driver] Fix integrated_as definition by setting it as a DriverOption

dblaikie added a comment. Is there missing test coverage for this in some way? (how does this patch change the observable behavior of clang? I guess --help would change?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83648/new/ https://reviews.llv

[PATCH] D83648: [Driver] Fix integrated_as definition by setting it as a DriverOption

dblaikie added a comment. In D83648#2146472 , @pzheng wrote: > Actually, this patch won't change --help because it just reduced some > duplication by extracting the common part (" the integrated assembler") of > the help message into the "help" of "OptOu

[PATCH] D83652: Merge some of the PCH object support with modular codegen

dblaikie created this revision. dblaikie added reviewers: rnk, llunak, hans. Herald added a project: clang. Herald added a subscriber: cfe-commits. I was trying to pick this up a bit when reviewing D48426 (& perhaps D69778 ) - in

[PATCH] D76646: Rename/refactor isIntegerConstantExpression to getIntegerConstantExpression

This revision was automatically updated to reflect the committed changes. Closed by commit rG49e5f603d400: Rename/refactor isIntegerConstantExpression to getIntegerConstantExpression (authored by dblaikie). Herald added a subscriber: sstefan1. Changed prior to commit: https://reviews.llvm.org/D

[PATCH] D83652: Merge some of the PCH object support with modular codegen

dblaikie updated this revision to Diff 277449. dblaikie added a comment. Include all the commits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83652/new/ https://reviews.llvm.org/D83652 Files: clang/include/clang/AST/ExternalASTSource.h clang/

[PATCH] D83652: Merge some of the PCH object support with modular codegen

dblaikie added a comment. In D83652#2146732 , @llunak wrote: > The patch is incomplete, isn't it? It removes DeclIsFromPCHWithObjectFile(), > but it's still called from ASTContext::DeclMustBeEmitted(). The description > also mentions updating of the pch-

[PATCH] D83702: [AIX]Generate debug info for static init related functions

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:248-250 CGF.StartFunction(GlobalDecl(&VD, DynamicInitKind::AtExit), -CGM.getContext().VoidTy, fn, FI, FunctionArgList()); +CGM.getContext().VoidTy, fn, FI, Funct

[PATCH] D83611: [clang][NFC] Add 'override' keyword to virtual function overrides

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! (I can't actually find where the instructions are on how to request commit access these days - but should you find them, I thin it'd be fine for you to have commit ac

[PATCH] D83702: [AIX]Generate debug info for static init related functions

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:248-250 CGF.StartFunction(GlobalDecl(&VD, DynamicInitKind::AtExit), -CGM.getContext().VoidTy, fn, FI, FunctionArgList()); +CGM.getContext().VoidTy, fn, FI, Funct

[PATCH] D83611: [clang][NFC] Add 'override' keyword to virtual function overrides

dblaikie added a comment. In D83611#2151208 , @logan-5 wrote: > Committed as rG2c2a297bb6d1 > If you add/leave the "Differential Revision: https://reviews.llvm.org/D"; line in th

[PATCH] D83702: [AIX]Generate debug info for static init related functions

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

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

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:103-118 + case Decl::CXXRecord: // struct/union/class X; [C++] +if (CGDebugInfo *DI = getDebugInfo()) + if (CGM.getCodeGenOpts().hasMaybeUnusedDebugInfo() && + cast(D).hasDefinition()) +

[PATCH] D82617: Disable GCC's -Woverloaded-virtual, which has false positives.

dblaikie added a comment. In D82617#2117441 , @sammccall wrote: > In D82617#2117086 , @Quuxplusone > wrote: > > > FWIW, I think the example you gave is **correct** for GCC to warn on. > > > Everything the warning s

[PATCH] D82617: Disable GCC's -Woverloaded-virtual, which has false positives.

dblaikie added a comment. In D82617#2118253 , @sammccall wrote: > In D82617#2118118 , @dblaikie wrote: > > > In D82617#2117441 , @sammccall > > wrote: > > > > > In D82617#21

[PATCH] D82617: Disable GCC's -Woverloaded-virtual, which has false positives.

dblaikie added a comment. In D82617#2119138 , @sammccall wrote: > > Guess perhaps a different question: Why don't you want this for clangd? > > Does it make the codebase better by not adhering to this particular warning? > > Yes, exactly. (Sorry if this w

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

dblaikie added subscribers: tamur, cmtice, JDevlieghere, labath, probinson, aprantl. dblaikie added a comment. In D76801#1989641 , @sammccall wrote: > Sorry about the problems here, and thanks for letting me know... > > In D76801#1989421

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

dblaikie added a comment. In D76801#1991904 , @labath wrote: > David's example does work with gdb without -Wl,--gdb-index (the member > variable is shown), presumably due to the aforementioned fuzzy matching. > However, it does not work if gdb-index is e

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

dblaikie added a comment. In D76801#1994003 , @labath wrote: > In D76801#1993337 , @dblaikie wrote: > > > In D76801#1991904 , @labath wrote: > > > > > David's example does wo

[PATCH] D78567: C++2a -> C++20 in some identifiers; NFC

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Herald added a subscriber: wuzish. Sounds good! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78567/new/ https://reviews.llvm.org/D78567 ___

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

dblaikie added a comment. In D76801#1996376 , @labath wrote: > In D76801#1995058 , @dblaikie wrote: > > > > It becomes a gdb-index problem because with an index the debugger will do > > > a (hashed?) string lookup

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dblaikie added a comment. > because it causes a 1% compile-time and memory usage regression. Yeah, some memory regression is expected and, in my opinion, acceptable for the change. The compile time regression presumably came from the changes to the report_fatal_error handling in SmallVector

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dblaikie added a comment. In D77621#1999757 , @browneee wrote: > I resubmitted the report_fatal_error checks again under D77601 > > > http://llvm-compile-time-tracker.com/compare.php?from=7375212172951d2fc283c81d03c1a8588

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

dblaikie accepted this revision. dblaikie added a comment. In D77621#2001099 , @dexonsmith wrote: > In D77621#2000237 , @nikic wrote: > > > Okay, I'm basically fine with that, if it is our stance that SmallVector >

[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:84 +template const size_t SmallVectorBase::SizeTypeMax; + nikic wrote: > Is this needed? I don't think it makes a lot of sense to allow odr-use of > `SizeTypeMax`. As it's a prote

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

dblaikie accepted this revision. dblaikie added a comment. Seems good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 ___ cfe-commits mailing list cfe-co

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

dblaikie added a comment. In D76801#2005264 , @labath wrote: > In D76801#1997451 , @dblaikie wrote: > > > Yeah, points all taken - as for this actual issue... I'm kind of inclined > > to say "hey, our template name

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