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

dblaikie added a comment. In D101566#2891923 , @aaronpuchert wrote: > In D101566#2891764 , @dblaikie > wrote: > >> This patch is still conflating two things - effectively removing an existing >> warning (which

[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#2897515 , @jmorse wrote: > David wrote: > >> think what I'm missing here: If -fno-standalone-debug is already in use/the >> default and is causing missing types because parts of the program are bulit >> without debug

[PATCH] D106582: [DebugInfo] Add -fno-ctor-homing for as counterpart to -fuse-ctor-homing

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks alright to me. (bit awkward having a documented cc1 option - since the cc1 options aren't meant to be user facing - hadn't really thought about/noticed that when reviewing the docume

[PATCH] D106582: [DebugInfo] Add -fno-ctor-homing for as counterpart to -fuse-ctor-homing

dblaikie added a comment. In D106582#2897678 , @akhuang wrote: > In D106582#2897599 , @dblaikie > wrote: > >> Looks alright to me. (bit awkward having a documented cc1 option - since the >> cc1 options aren't me

[PATCH] D106585: Fix clang debug info irgen of i128 enums

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117 +llvm::APSInt Value = Enum->getInitVal(); +Value.setIsSigned(IsSigned); +Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value)); Is the value already

[PATCH] D106585: Fix clang debug info irgen of i128 enums

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117 +llvm::APSInt Value = Enum->getInitVal(); +Value.setIsSigned(IsSigned); +Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value)); rnk wrote: > dblaikie

[PATCH] D106585: Fix clang debug info irgen of i128 enums

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117 +llvm::APSInt Value = Enum->getInitVal(); +Value.setIsSigned(IsSigned); +Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value)); rnk wrote: > rnk wrot

[PATCH] D106701: [clang] Add -falign-loops=N where N is a power of 2

dblaikie added a comment. In D106701#2901656 , @MaskRay wrote: > In D106701#2901639 , @efriedma > wrote: > >> Can we hook this up to a LLVM IR function attribute, instead of making it a >> codegen flag? > > The

[PATCH] D106585: Fix clang debug info irgen of i128 enums

dblaikie accepted this revision. dblaikie added a comment. Preserving existing behavior sounds OK - maybe some comment about that it might be good to remove so the next person who looks at it knows there's something not-quite-fully-reasoned here (& the comment about GCC's representation choice

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

dblaikie added a comment. Just a thought (nothing to hold up this patch/suggest a revert/suggest any immediate action), but: > The problem with extending this to non-system headers is that you need a way > to tell which headers are allowed to include the detail headers and which > ones are not

[PATCH] D106585: Fix clang debug info irgen of i128 enums

dblaikie added a comment. In D106585#2902588 , @dblaikie wrote: > Preserving existing behavior sounds OK - maybe some comment about that it > might be good to remove so the next person who looks at it knows there's > something not-quite-fully-reasoned h

[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 a comment. Sounds like this is good for Google and Sony, and Apple doesn't use `-fno-standalone-debug`/`-flimit-debug-info` anyway, so probably about ready to move forward, then? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

dblaikie added a comment. In D106394#2910571 , @cjdb wrote: > In D106394#2905660 , @dblaikie > wrote: > >> Just a thought (nothing to hold up this patch/suggest a revert/suggest any >> immediate action), but: >>

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie added a comment. In D118511#3369114 , @tbaeder wrote: > Hey @dblaikie, seems like this has never been pushed? Yeah, was holding off on this because it looked like maybe there was still outstanding work on the nuance/precise nature of the ABI ch

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

dblaikie created this revision. dblaikie added reviewers: aaron.ballman, rsmith, denik, deansturtevant. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Some functions can end up non-externally visible d

[PATCH] D120610: [DebugInfo] Include DW_TAG_skeleton_unit when looking for parent UnitDie

dblaikie added a comment. Fixes in LLVM require tests in LLVM - probably taking the clang test and compiling that to llvm IR (include the original C++ source in a comment in the IR test case) and then testing it in LLVM instead of clang. Also looks like the test could be simplified a bit more:

[PATCH] D121100: [clang][DebugInfo] clang should not generate DW_TAG_subprogram entry without DW_AT_name

dblaikie added a comment. Yeah, happy to hear other perspectives - but my rough reaction is similar: putting mangled names in the "name" field seems problematic (consumer wouldn't necessarily know that the name should be demangled, for instance? Maybe?). So at the IR level maybe it's better to

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie added a comment. In D118511#3372728 , @jyknight wrote: > In D118511#3371432 , @tstellar > wrote: > >> I'm fine with reverting if you think this is the best solution. I just >> would like to conclude so

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie added a comment. In D118511#3373173 , @tstellar wrote: > In D118511#3373082 , @dblaikie > wrote: > >> In D118511#3372728 , @jyknight >> wrote: >> >>> In D118511

[PATCH] D120610: [DebugInfo] Include DW_TAG_skeleton_unit when looking for parent UnitDie

dblaikie added a comment. It'd be good to include some testing beyond "does not crash" - like what was the specific debug info we were trying to create when the crash was hit? Perhaps we should be testing that (since the crash demonstrates we weren't testing that anywhere else) Repository:

[PATCH] D120610: [DebugInfo] Include DW_TAG_skeleton_unit when looking for parent UnitDie

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

[PATCH] D122046: [clang] Remove Address::deprecated from MveEmitter

dblaikie added inline comments. Comment at: clang/utils/TableGen/MveEmitter.cpp:1197 +const Type *Ty = nullptr; +if (auto *DI = dyn_cast(D->getArg(0))->getOperator()) + if (auto *PTy = dyn_cast(getType(DI, Param))) nikic wrote: > Should be either `ca

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

dblaikie added inline comments. Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2 +// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify +// expected-no-diagnostics +export module m; ChuanqiXu wrote: > urnathan wrote: > > dblaikie wrote: > > > Thi

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

dblaikie added a comment. In D119409#3334537 , @ChuanqiXu wrote: > In D119409#3332313 , @dblaikie > wrote: > >> (maybe relevant: For what it's worth: I originally implemented inline >> function homing in modules

[PATCH] D122046: [clang] Remove Address::deprecated from MveEmitter

dblaikie added inline comments. Comment at: clang/utils/TableGen/MveEmitter.cpp:1197 +const Type *Ty = nullptr; +if (auto *DI = dyn_cast(D->getArg(0))->getOperator()) + if (auto *PTy = dyn_cast(getType(DI, Param))) aeubanks wrote: > simon_tatham wrot

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

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

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

dblaikie added a comment. In D121328#3396984 , @aaron.ballman wrote: >> once we figure out what to do about the change in behavior for >> -Wnon-c-typedef-for-linkage > > The devil is in the details; I'm not sure what to do here. I don't think > there's

[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

dblaikie added inline comments. Comment at: clang/test/CodeGenCXX/inconsistent-export-template.cpp:1-11 +// RUN: %clang_cc1 -std=c++20 %s -S -emit-llvm -triple %itanium_abi_triple -disable-llvm-passes -o - | FileCheck %s + +export module m; +export template +void f() { +} + ---

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

dblaikie added a comment. SOrry, I don't have much context here - the more informative (module/internal linkage) diagnostic does seem better to me than saying "is not exported", even if it's a bit esoteric for some users. We do have other diagnostics that mention linkage, I'm sure (because it's

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

dblaikie added a comment. In D119409#3398483 , @ChuanqiXu wrote: > In D119409#3396690 , @dblaikie > wrote: > >> Not even necessarily then - if you have code like protobufs (large amounts >> of autogenerated code

[PATCH] D120989: Support debug info for alias variable

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D120989#3411553 , @kavitha-natarajan wrote: >> Thanks for the details - can you ping this thread once the gdb thread has >> progressed/seems l

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

dblaikie accepted this revision. dblaikie added a comment. In D122119#3432220 , @iains wrote: > @dblaikie - is the explanation for the change in diagnostics at the same time > OK? (if not, I am happy to split the patch, but either way I'd like to land >

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

dblaikie added a comment. (@probinson as someone I've disagreed with about this before) Personally I think there's limited value in expressing 'auto' in DWARF at all - we could omit function declarations if the return type is not known (undeduced auto) and wouldn't lose much - basically treatin

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. This seems like it would significantly introduce debug info size for at least some kinds of code - have you done any size measurements of this change? What does the resulting DWARF look like? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D121959: [clang] Add missing diagnostics for invalid overloads of multiversion functions in C.

dblaikie added a comment. This might've caused a regression? (https://github.com/llvm/llvm-project/issues/54892) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121959/new/ https://reviews.llvm.org/D121959 __

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

dblaikie added a comment. In D123345#3453037 , @rnk wrote: > Generally speaking, this sounds like a good idea to me. One time in 2019 I > used -ftime-trace+ClangBuildAnalyzer and it told me that std::unique_ptr was > the most expensive template >

[PATCH] D121959: [clang] Add missing diagnostics for invalid overloads of multiversion functions in C.

dblaikie added a comment. In D121959#3454606 , @tahonermann wrote: >> This might've caused a regression? >> (https://github.com/llvm/llvm-project/issues/54892) > > @dblaikie, it most definitely did. Note that I'm the author of the patch that > introduc

[PATCH] D124006: [DebugInfo] Use the 'getTypeAlignIfRequired' function to get DW_AT_alignment correct when attribute((__aligned__)) is present.

dblaikie added a comment. Could you include some static_asserts(+alignof) in the test case to demonstrate these are the alignments that the language is computing too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124006/new/ https://reviews.llvm.o

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. In D123534#3454749 , @hctim wrote: > In D123534#3454354 , @dblaikie > wrote: > >> This seems like it would significantly introduce debug info size for at >> least some kinds of code - h

[PATCH] D123436: [Clang] Pass llvm::BitstreamCursor by reference. NFC

dblaikie added a comment. Perhaps GlobalModuleIndex should create the cursor itself - it's being handed the buffer anyway? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123436/new/ https://reviews.llvm.org/D123436

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie updated this revision to Diff 405468. dblaikie added a comment. - Create a separate sub-warning flag of -Wpacked (-Wpacked-non-pod), in case you're just interested in the ABI breaks here - Warn only when this produces a different alignment requirement for the field Repository: rG LLV

[PATCH] D118781: Reduce dependencies on llvm/BinaryFormat/Dwarf.h

dblaikie added a comment. after: 10978519 before: 11245451 Doesn't appear to be a /huge/ win to me... but I don't mind too much. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118781/new/ https://reviews.llvm.org/D118781 __

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

dblaikie added a comment. In D117616#3295859 , @Bhramar.vatsa wrote: > @dblaikie > The condition `FieldClass->isPOD()` returns false for the following case > (when considering field 'struct foo t' of 'struct foo1') : > > class foo { > foo() = de

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

dblaikie created this revision. dblaikie added reviewers: rsmith, Bhramar.vatsa, rjmccall, rnk. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D119051 Files: clang

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

dblaikie added a comment. Hmm, I guess it might be the C++11 definition, as suggested - since a base class (even a public one) seems to classify the type as "non pod" as far as GCC is concerned ( In D117616#3298001 , @dblaikie wrote: > In D117616#32958

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

dblaikie added a comment. Test case showing GCC's behavior here: https://godbolt.org/z/4W7j8Yd54 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 ___ cfe-commits

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

dblaikie added a comment. In D119051#3298491 , @dblaikie wrote: > Test case showing GCC's behavior here: https://godbolt.org/z/4W7j8Yd54 Oh, and demonstrating this applies (as best as I can figure) even when compiling in C++03 mode: https://godbolt.org/

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie updated this revision to Diff 406626. dblaikie added a comment. Refactor the max/min/preferred alignment calculations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118511/new/ https://reviews.llvm.org/D118511 Files: clang/include/clang

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

dblaikie marked an inline comment as done. dblaikie added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036 // The align if the field is not packed. This is to check if the attribute // was unnecessary (-Wpacked). CharUnits UnpackedFieldAlign =

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

dblaikie updated this revision to Diff 406628. dblaikie added a comment. Remove FieldClass check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/lib/AST/RecordLayoutBuilder.cpp clang/test

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

dblaikie added a comment. In D119051#3302125 , @rnk wrote: > Looks like the test fails on the Windows pre-merge bot: > https://buildkite.com/llvm-project/premerge-checks/builds/77696#1836f181-a998-4695-b587-a83239ea > > The debian bot seems to be fai

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

dblaikie updated this revision to Diff 406904. dblaikie added a comment. - Change the definition of C++03 POD to include defaulted functions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

dblaikie added a comment. Yeah, looks like GCC believes `cxx11_pod::t1` to be C++03 POD even though it doesn't quite follow the letter of the standard - but since teh "= default" is an extension, I guess they get to define what that extension means. Here's an example: struct t1 { int i; };

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

dblaikie added a comment. In D119051#3314138 , @Bhramar.vatsa wrote: > Sorry, but I can only add a bit more confusion: > https://godbolt.org/z/fT19KTh34 > There are two cases, only differing in terms of user-defined constructor. > > Gcc and clang differ

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

dblaikie added a comment. Thanks for chiming in! In D119051#3315741 , @rjmccall wrote: > Changing the C++03 POD definition is going to be substantially ABI-breaking > at this point. Any logic to change it needs to be platform-specific. Even if it's on

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

dblaikie added a comment. In D119051#3315747 , @rjmccall wrote: > And this should be raised as an Itanium issue. Ah, looks like this is the existing https://github.com/itanium-cxx-abi/cxx-abi/issues/66 Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

dblaikie added a comment. In D105169#3315009 , @MaskRay wrote: > It may not be worth changing now, but I want to mention: it's more > conventional to have a `BoolOption` which adds `-[no-]noundef-analysis`. > Since both positive and negative forms exist

[PATCH] D119409: [C++20] [Modules] Remain variable's definition in module interface

dblaikie added inline comments. Comment at: clang/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp:5 // CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global -// CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global +// CHECK-DAG: @inline_var_e

[PATCH] D124006: [DebugInfo] Use the 'getTypeAlignIfRequired' function to get DW_AT_alignment correct when attribute((__aligned__)) is present.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good, thanks! Comment at: clang/test/CodeGenCXX/debug-info-struct-align.cpp:11 +} __attribute__((aligned(1))); +struct MyType mt; + You can drop th

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

dblaikie added a comment. ('scuse the delay) Baseline: I'm still not really sure this is the right direction. Is there a sound argument for why this change is suitable for lambdas, but not for other types? I believe all the situations that can happen with other types can happen with lambdas (&

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. Size still seems moderately concerning, but might be acceptable. Got a summary of what the DWARF looks like now (without names)? (maybe there's something else we can strip/optimize) & how many of these descriptions get added to the debug info? Numbers for Split DWARF

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

dblaikie added a comment. In D123319#3473693 , @shafik wrote: > In D123319#3473283 , @dblaikie > wrote: > >> ('scuse the delay) >> >> Baseline: I'm still not really sure this is the right direction. Is there a >

[PATCH] D123436: [Clang] Use std::move in GlobalModuleIndex::readIndex. NFC

dblaikie added a comment. In D123436#3462567 , @dblaikie wrote: > Perhaps GlobalModuleIndex should create the cursor itself - it's being handed > the buffer anyway? Ping on this ^ - would this be a better direction that addresses the concerns? Reposit

[PATCH] D124434: [Clang][Test] Run tests in C++14 mode explicitly.

dblaikie added subscribers: aaron.ballman, rsmith. dblaikie added a comment. @rsmith @aaron.ballman - might be especially interesting to know your thoughts on the C++ chapter-based testing and what the intent there is as clang changes default versions/new versions are added. (& also whether ther

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. In D123534#3475790 , @hctim wrote: >> summary of DWARF: >> & how many of these descriptions get added to the debug info? > > afaict, there is now: > 1x .debug_addr entry for each string > 1x. debug_info DW_TAG_variable for each

[PATCH] D123436: [Clang] Use std::move in GlobalModuleIndex::readIndex. NFC

dblaikie added a comment. In D123436#3475858 , @junaire wrote: > In D123436#3475002 , @dblaikie > wrote: > >> In D123436#3462567 , @dblaikie >> wrote: >> >>> Perhaps Glo

[PATCH] D124434: [Clang][Test] Run tests in C++14 mode explicitly.

dblaikie added a comment. In D124434#3479828 , @aaron.ballman wrote: > In D124434#3479051 , @junaire wrote: > >>> In general, my concern with the this patch is that it loses test coverage >>> by specifying an ex

[PATCH] D124434: [Clang][Test] Run tests in C++14 mode explicitly.

dblaikie added a comment. In D124434#3480519 , @aaron.ballman wrote: > In D124434#3480481 , @dblaikie > wrote: > >> Yeah, I have mixed feelings - I think at least in theory, C++ tries to be >> mostly backwards

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

dblaikie added a comment. Would an explicit naming be more suitable than a region start/end? (I'd have considered this feedback for D116503 too, but didn't catch that one in review) The region based thing makes non-positional arguments weirdly positional (not

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

dblaikie added a comment. In D123319#3474997 , @dblaikie wrote: > In D123319#3473693 , @shafik wrote: > >> In D123319#3473283 , @dblaikie >> wrote: >> >>> ('scuse the del

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added subscribers: aprantl, rnk. dblaikie added a comment. Tried this patch on some internal targets and metrics and it seems actually quite reasonable. Tested with split DWARF, on Linux, with compressed debug info in .o/.dwo files, uncompressed in exe/dwp files. With -O0 and -O3, roug

[PATCH] D125693: [DebugInfo] Support types, imports and static locals declared in a lexical block (3/5)

dblaikie added a subscriber: JDevlieghere. dblaikie added a comment. In D125693#3641742 , @krisb wrote: > @dblaikie, could you please take a look at this and/or D113741 > ? Do you see any ways to proceed? My concern wit

[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

dblaikie added a comment. > hmm there seems to be a compiler error, which looks somewhat unrelated to the > active patch: > > > /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp:125:27: > error: n

[PATCH] D125693: [DebugInfo] Support types, imports and static locals declared in a lexical block (3/5)

dblaikie added a comment. In D125693#3648942 , @krisb wrote: > In D125693#3644029 , @dblaikie > wrote: > >> In D125693#3641742 , @krisb wrote: >> >>> @dblaikie, could you

[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] D130516: [Support] compression classes

dblaikie added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:2003-2004 // consumers will not want its contents. + llvm::compression::CompressionAlgorithm CompressionScheme = + llvm::compression::ZlibCompressionAlgorithm(); + Doesn't

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

dblaikie added a comment. Any chance this could be a generalization of https://clang.llvm.org/doxygen/Consumed_8cpp_source.html ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130055/new/ https://reviews.llvm.org/D130055 _

[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] D130516: [Support] compression classes

dblaikie added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:2003-2004 // consumers will not want its contents. + llvm::compression::CompressionAlgorithm CompressionScheme = + llvm::compression::ZlibCompressionAlgorithm(); + ckissane

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

dblaikie added a comment. In D123319#3486544 , @dblaikie wrote: > In D123319#3474997 , @dblaikie > wrote: > >> In D123319#3473693 , @shafik wrote: >> >>> In D123319#34732

[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

dblaikie added inline comments. Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1044 + + // Unfortunately, debug_aranges by default don't inclue global variables. If + // we failed to find the CU using aranges, try and search for variables as Might be wor

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie accepted this revision. dblaikie added a comment. Seems good - if folks find the growth too much, or it creates weird perf issues otherwise, we can discuss that if/when it comes up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/

[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

dblaikie added inline comments. Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:769-783 +DataExtractor Data(Location.Expr, /*IsLittleEndian=*/true, 8); +uint64_t DataOffset = 0; +uint8_t Operation = Data.getU8(&DataOffset); +if (Operation == dwarf::DW_OP_addr)

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

dblaikie added a comment. In D123319#3506745 , @shafik wrote: >> Any update/further details here? > > David, apologies for not getting back to you sooner. The context is D105564 > which I started working on again recent

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. Might be worth a note in the Release Notes given that it's at least caused issues with one DWARF consumer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 __

[PATCH] D123538: [symbolizer] Parse DW_TAG_variable DIs to show line info for globals

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems OK Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1040-1041 // Retrieve the compile unit. - return getCompileUnitForOffset(CUOffset); + DWARFCompileUnit

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

dblaikie added inline comments. Comment at: clang/docs/ReleaseNotes.rst:239-243 +- GCC doesn't pack non-POD members in packed structs unless the packed + attribute is also specified on the member. Clang historically did perform + such packing. Clang now matches the gcc behavior

[PATCH] D126224: Add DWARF string debug to clang release notes.

dblaikie added a comment. In D126224#3531949 , @rnk wrote: > I am reminded of the perennial problem of "optional" protobuf fields that, > when omitted, will cause production crashes. > > Do you think it would be less disruptive to synthesize a name? I be

[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] D66830: Consumed checker: various improvements

dblaikie added a comment. Likely best to separate these changes into separate/standalone patches - easier to review/see what's changing, what the motivation is, etc. (I'm probably OK with some breakage here - to the best of my knowledge these attributes haven't achieved widespread adoption, so

[PATCH] D65371: do not emit -Wunused-macros warnings in -frewrite-includes mode (PR15614)

dblaikie added a comment. A test case would be good (in the clang/test directory - probably near/in the other tests for -frewrite-includes) And does the same bug occur for other preprocessor-related warnings? Maybe it's not practical to disable them all this way & there should be a different s

[PATCH] D65371: do not emit -Wunused-macros warnings in -frewrite-includes mode (PR15614)

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Fair enough - thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65371/new/ https://reviews.llvm.org/D65371 _

[PATCH] D67373: Don't emit .gnu_pubnames when tuning for LLDB

dblaikie added a comment. Have you got a link to the original thread where this was discussed/I mentioned it? Want to page in some context to double-check if I had any ideas that might've let this simplify things. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D67373: Don't emit .gnu_pubnames when tuning for LLDB

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

[PATCH] D36474: Use the file name from linemarker for debug info if an input is preprocessed source.

dblaikie added a comment. Looks plausible (not sure how nice it is to use the llvm::Module itself to communicate the file name to this bit of logic - I'll leave that to Richard Smith to judge, I think) Comment at: test/CodeGen/debug-info-preprocessed-file.i:10 +// RUN: %clang

[PATCH] D36386: [clang] Remove unit test which uses reverse-iterate flag

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. sounds good - so long as other tests would fail if the fix this test was testing wasn't present (on a reverse iteration enabled build) https://reviews.llvm.org/D36386 _

[PATCH] D71026: Fix LLVM_ENABLE_MODULES=ON + BUILD_SHARED_LIBS=ON build

dblaikie added a comment. In D71026#1785025 , @arichardson wrote: > If I build on macOS with `cmake -GNinja -DBUILD_SHARED_LIBS=ON > -DCMAKE_BUILD_TYPE=Debug > -DLLVM_ENABLE_PROJECTS=llvm;clang;lld;compiler-rt;libcxx;libcxxabi > -DBUILD_SHARED_LIBS=ON -

[PATCH] D69779: -fmodules-codegen should not emit extern templates

dblaikie added a comment. Hey - thanks for this! Does look like it reproduces in modules: foo.h: #pragma once template struct outer { static void func() { } }; template void func() { } extern template struct outer; extern template void func(); inline void cal

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