[PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

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

[PATCH] D135488: [codegen] Display stack layouts in console

dblaikie added a comment. > as a more general diagnostic output, like the other printing passes. As an aside: I don't think any "printing pass" is designed to be used beyond LLVM compiler engineers - they're implementation details of the compiler, even/much moreso than the optimization remarks

[PATCH] D135488: [codegen] Display stack layouts in console

dblaikie added a comment. Any chance we could squirrel the info away (I assume there's a reason we can't compute the info where the warn-stack-size LLVM feature is implemented in PrologEpilogInserter.cpp) somewhere, and emit it as part of the frame-larger-than/warn-stack-size diagnostic?

[PATCH] D137058: [Driver] [Modules] Support -fsave-std-c++-module-file (1/2)

dblaikie added inline comments. Comment at: clang/lib/Driver/Driver.cpp:5541-5542 + // + // FIXME: Do we need to handle the case that the calculated output is + // conflicting with the specified output file or the input file? + if (!AtTopLevel && isa(JA) &&

[PATCH] D137885: [modules] Support zstd in .pcm file

dblaikie added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:1456 + const llvm::compression::Format F = + Blob.size() >= 2 && memcmp(Blob.data(), "\x1f\x8b", 2) == 0 + ? llvm::compression::Format::Zlib Worth a

[PATCH] D137609: [C++20] [Modules] Remove unmaintained header modules

dblaikie added a comment. > I'm not aware of anyone using this mode, but please wait for responses from > Google and Meta people to verify that. Best understanding on the Google side is we aren't using this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137609/new/

[PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

dblaikie added a comment. In D137583#3917706 , @aaron.ballman wrote: >> ...we expect template params to be fully qualified when comparing them for >> simple template names > > So lldb is not inspecting the AST, they're doing reflection (of a sort) on

[PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

dblaikie added a subscriber: aaron.ballman. dblaikie added a comment. @aaron.ballman does this seem OK? (this was based on my suggestion in the related review linked in the description) It probably needs tests in clang too - not sure if there's an opportunity to use a unit test to simplify

[PATCH] D136554: Implement CWG2631

dblaikie added a comment. Looks like it's distinct from the other reported crash, this one crashes with: clang-16: /usr/local/google/home/blaikie/dev/llvm/src/clang/lib/CodeGen/CGExpr.cpp:2811: clang::CodeGen::LValue clang::CodeGen::CodeGenFunction::EmitDeclRefLValue(const

[PATCH] D136554: Implement CWG2631

dblaikie added subscribers: rsmith, dblaikie. dblaikie added a comment. fwiw, @rsmith came up with a crasher reproducer from this patch here: template struct F { template F(const U&) {} }; struct A { static constexpr auto x = [] {}; F f = x; }; void f(A a = A())

[PATCH] D137058: [Driver] [Modules] Support -fsave-std-c++-module-file (1/2)

dblaikie added a comment. > What was the objection to "-fc++-module-filename[=]" ? I guess it reads a bit awkwardly when you aren't providing the filename/want the default filename? > GCC has "-fmodule-only" Hmm, I don't mind that too much (& as you say, '-fobject-only' - though that flag is

[PATCH] D137059: [Driver] [Modules] Introduce -fsave-std-c++-module-file= to specify the path of the module file (2/2)

dblaikie added a subscriber: rsmith. dblaikie added a comment. In D137059#3899339 , @ben.boeckel wrote: > There is another motivating factor for 1-phase: the build graph is far > simpler. With 2-phase, CMake will have to write out rules to perform: > >

[PATCH] D137058: [Driver] [Modules] Support -fsave-std-c++-module-file (1/2)

dblaikie added a subscriber: rsmith. dblaikie added a comment. I realize I got this jumbled up and the thread about "why do we need to name things" is meant to be over in D137059 (sorry @ben.boeckel :/ I know this is all confusing to follow at the best of

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

dblaikie added a comment. This is a bit out of left field, but these comments reminded me of something - a long time ago I was working on fixing some Clang layering to potentially use explicit modules more in our internal build (& then hopefully in the upstream/external build) and one major

[PATCH] D137058: [Driver] [Modules] Support -fsave-std-c++-module-file (1/2)

dblaikie added a comment. @ben.boeckel maybe this'd be the place to discuss the motivation for this feature (picking up from your comment here: https://reviews.llvm.org/D134267#3892629 ) Could you expound a bit on why "write the .pcm to the same path as the .o" isn't sufficient? (like Split

[PATCH] D137059: [Driver] [Modules] Introduce -fsave-std-c++-module-file= to specify the path of the module file (2/2)

dblaikie added a comment. Could you link to the email/discourse discussion about supporting this mode (I think you've linked it in other discussions, be good to have it for reference here & Probably in the other review)? (I'm wondering if we need a new flag for this, or if it'll be OK to

[PATCH] D137067: [DebugInfo][Metadata] Make AllEnumTypes holding TrackingMDNodeRef

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Test case can be simplified a bit further: template struct Struct1 { enum { enumValue1 }; Struct1(); }; void function2() { struct Struct3 {}; int i =

[Diffusion] rGe4ec6ce8a75c: Clang: Add release note for defaulted-special-members-POD GCC ABI fix

dblaikie marked an inline comment as done. dblaikie added inline comments. BRANCHES main /clang/docs/ReleaseNotes.rst:612-613 Oh, sure: 97c1b0094ad6f628927223b53300c9296ae72f44 Users: dblaikie (Author) https://reviews.llvm.org/rGe4ec6ce8a75c

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

dblaikie added a comment. In D119051#3888667 , @rnk wrote: > I realized I forgot some things I should've mentioned: > > - This probably deserves a release note, if it doesn't have one already that > I missed or forgot about

[PATCH] D136807: [clang][Sema] Fix a clang crash with btf_type_tag

dblaikie added a comment. Could the test be expanded to test more specifically than "does not crash"? Like is there some part of the output that can't be observed in any other test (that didn't/doesn't crash without this patch) - like the location emitted in some debug info, etc? Repository:

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGec273d3e3a8c: Add a warning for not packing non-POD members in packed structs (authored by dblaikie). Changed prior to commit:

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7846d590033e: Extend the C++03 definition of POD to include defaulted functions (authored by dblaikie). Changed prior to commit:

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. >>> So we might print it with a syntax as if the class was a simple pod type, >>> even though that might produce a completely different result if used in >>> practice. >>> I don't think we can do much better without having the actual expression >>> used. >>> >>> That

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. > it's up to IDE's and good editors and CI log parsers to parse that and > present in a possibly better format. Lots of folks use the diagnostic info from their compiler without IDEs or log parsers doing anything else to them - we should be trying to design these

[PATCH] D135583: [LLVM] Use DWARFv4 bitfields when tuning for GDB

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

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. In D134453#3880525 , @mizvekov wrote: > In D134453#3880403 , @dblaikie > wrote: > >> Could you describe this in more detail? I find that to be more verbose than >> is necessary/helpful

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. In D134453#3877127 , @mizvekov wrote: > I think the type printer's purpose is for printing types for diagnostics, not > for generating a lossless representation of types as strings, as the ast > dumper does. > So packing so

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

dblaikie added a comment. Ping (hoping to ensure enough progress is made on this that we can get it in before the LLVM 16 branch - since it's complicated the last couple of releases already due to not having all the fallout of the ABI fixes in together) Repository: rG LLVM Github Monorepo

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. In D85802#3880078 , @mcgrathr wrote: > In D85802#3879950 , @dblaikie wrote: > >> In D85802#3879888 , @mcgrathr wrote: >> >>> In D85802#3876106

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. In D85802#3879888 , @mcgrathr wrote: > In D85802#3876106 , @dblaikie wrote: > >>> The C++ ABI is not part of the Fuchsia system ABI, nor what we call the >>> "Fuchsia compiler ABI".

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. > The C++ ABI is not part of the Fuchsia system ABI, nor what we call the > "Fuchsia compiler ABI". Different users of C++ are free to use whatever C++ > ABI they like. Only the backend ABI independent of language-specific issues > is necessary to interoperate with

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. I'm getting a bit exhausted with all the words involved here & not sure how to simplify/clarify this. If @ben.boeckel has particular use cases, it might be easier for him to be here discussing them so we can discuss the tradeoffs directly rather than through

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. In D134453#3875392 , @erichkeane wrote: > FWIW, i find the GCC diagnostics (and the application of this patch) to be > much more clear/easy to read. The pile of `{` and `}` don't really look > useful/readable/meaningful to

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. >> (I'd still sort of lean towards "make it the same as the .o, but with the >> .pcm suffix" and if the build system wants to put things in other places, it >> can move them around - but I understand why that's not something everyone's >> on board with) > > Actually,

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. > you would get something like `S int, Length>{.value = 50}}, .width = Width{StrongTypedef Width>{.value = 70}}}>` (inspired by this GCC output), which is truly > verbose. However, the current way of printing (assuming member names are > printed) it would print

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added a comment. In D135557#3871803 , @aaron.ballman wrote: > In D135557#3871716 , @dblaikie > wrote: > >> (I'm still sort of curious how the AST matchers deal with all this - I guess >> they must

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added a comment. In D135557#3871622 , @aaron.ballman wrote: > In D135557#3871482 , @dblaikie > wrote: > >> I was hoping the rephrasing (is this really a question about which ctors the >> type has, or

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added a comment. (pulling this out of inline comments because Phab's reply quoting doesn't work so well there & the threading/ordering gets weird too) >> Well, I'm hoping we can find a way to avoid doing that in the general case >> while still giving users a way to opt into that

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. In D134453#3871458 , @dblaikie wrote: > In D134453#3871414 , @aaron.ballman > wrote: > >> In D134453#3871386 , @dblaikie >> wrote: >> In

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. In D134453#3871414 , @aaron.ballman wrote: > In D134453#3871386 , @dblaikie > wrote: > >>> In terms of next steps, I think we should try to see if there's a "clear" >>> path forward

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration aaron.ballman wrote: > anderslanglands

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. > In terms of next steps, I think we should try to see if there's a "clear" > path forward in terms of printing the types vs not printing the types. If we > find one, then great, we go that way. But if we don't seem to have a clear > path forward (relatively

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. @aaron.ballman: thanks for tracking down the source of confusion between our perspectives. (& yeah, sorry, could've included less ambiguous examples with the extra nesting so we'd have avoided all that confusion) Do you have particular examples where you find the

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3869643 , @iains wrote: > In D134267#3869614 , @dblaikie > wrote: > >> In D134267#3869162 , @iains wrote: >> >>> In D134267#3868830

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3869162 , @iains wrote: > In D134267#3868830 , @dblaikie > wrote: > >> I'm OK with sticking with the existing `-fmodule-file` if that works for >> everyone. Yeah, it's short

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. In D134453#3869201 , @aaron.ballman wrote: > In D134453#3868821 , @DoDoENT wrote: > >> In D134453#3868789 , @dblaikie >> wrote: >> >>> I still

[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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. I'm OK with sticking with the existing `-fmodule-file` if that works for everyone. Yeah, it's short and ambiguous in a space with many concepts of what a "module file" is, but also the fact that it's a `-f` flag might help disambiguate it a bit - it's probably not the

[PATCH] D134453: Disambiguate type names when printing NTTP types

dblaikie added a comment. I still don't think "keep full NTTP type printing behind a policy, for those that want/need that" is a policy we should add. String printed names aren't meant to be a tool for type reflection - the AST can be queried for that information. Comment

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration aaron.ballman wrote: > dblaikie wrote: >

[PATCH] D136188: Update docs for -fuse-ctor-homing

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me with the addition of the forth homing strategy. In D136188#3866494 , @probinson wrote: > +jmorse who is closer to this topic

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration anderslanglands wrote: > dblaikie wrote:

[PATCH] D136188: Update docs for -fuse-ctor-homing

dblaikie added a subscriber: probinson. dblaikie added a comment. (Speaking of which, might be worth pushing on the changes to remove the flags/support for opting out of ctor homing... I know @probinson had some concerns - but I really don't want to end up holding onto the tech debt of these

[PATCH] D136188: Update docs for -fuse-ctor-homing

dblaikie added a comment. In D136188#3866385 , @akhuang wrote: > Actually, maybe I should add some of this info to the -fstandalone-debug > section. Ah, sure, if you like. At some point I think I/someone should write a full ramble about DWARF type

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration anderslanglands wrote: > dblaikie wrote:

[PATCH] D135192: Fix incorrect check for running out of source locations.

This revision was automatically updated to reflect the committed changes. Closed by commit rG5b773dcd2de0: Fix incorrect check for running out of source locations. (authored by ppluzhnikov, committed by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D135192: Fix incorrect check for running out of source locations.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good (guess this is pretty impractical to have a regression test/would require very large files to test?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration aaron.ballman wrote: > dblaikie wrote: >

[PATCH] D136188: Update docs for -fuse-ctor-homing

dblaikie added a comment. (not sure whether to discuss this here or on the bug) Should we remove the documentation? Part of the principles of "cc1" flags is that their implementation details, not to be publicly used and maybe not to be publicly documented either? Repository: rG LLVM Github

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

dblaikie accepted this revision. dblaikie added a comment. In D136041#3864171 , @yonghong-song wrote: > In D136041#3863748 , @dblaikie > wrote: > >> Hmm - this does mean linking IR can produce invalid code,

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3851479 , @ChuanqiXu wrote: > In D134267#3849629 , @dblaikie > wrote: > >> To the original point I was making about implicit modules (which might've >> been my own confusion

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. (sorry I've not been following all this as closely as I should - but I don't think BMIs are an implementation detail, anymore than object files are - users should/will be as aware of BMIs as they are of .o files - there build artifacts that need to be cached, can be

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

dblaikie added a comment. Hmm - this does mean linking IR can produce invalid code, though, right (you link in a definition of the function, so what was valid is now invalid - because it now has a definition, can be inlined, etc)? Is that new? concerning? Repository: rG LLVM Github Monorepo

[PATCH] D135713: [cmake][Fuchsia] Add -ftrivial-auto-var-init=zero to runtimes build

dblaikie added a comment. any chance of using pattern init rather than zero init, so this doesn't hide uninitialized bugs? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135713/new/ https://reviews.llvm.org/D135713

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

dblaikie added inline comments. Comment at: clang/test/CodeGen/bounds-checking-fam.c:2 // REQUIRES: x86-registered-target -// RUN: %clang_cc1 -emit-llvm -triple x86_64 -fstrict-flex-arrays=0 -fsanitize=array-bounds%s -o - | FileCheck %s

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. In D85802#3859540 , @leonardchan wrote: >> Naive question, but what does it mean to target Fuschia but be >> gnu-compatible? (how would that be different than using whatever gnu OS >> (linux, etc) the other code was built for)

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration anderslanglands wrote: > dblaikie wrote:

[PATCH] D135916: Itanium ABI: Pack non-pod members of packed types

This revision was automatically updated to reflect the committed changes. Closed by commit rG037f85668126: Itanium ABI: Pack non-pod members of packed types (authored by dblaikie). Changed prior to commit: https://reviews.llvm.org/D135916?vs=467599=467890#toc Repository: rG LLVM Github

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. In D85802#3859235 , @leonardchan wrote: > In D85802#3858760 , @rjmccall wrote: > >> Does Fuchsia still need this? If those experiments have stabilized, maybe >> we can just remove it.

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

dblaikie added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration aaron.ballman wrote: > royjacobson wrote:

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

dblaikie added a comment. Herald added a subscriber: abrachet. Herald added a project: All. FWIW, the existence of this flag and the TargetCXXABI abstraction (which predates the flag, but might otherwise be more simply factored/implemented with inheritance rather than switch tables) came up in

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a comment. In D135326#3854141 , @rjmccall wrote: > I don't remember the history of the `-fc++-abi` flag at all, so if that's a > driver flag, you'll probably need to investigate it more to remove it. Maybe > it was added for testing

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

dblaikie added a comment. Hmm - pinging this since my last comment didn't seem to send mail (at least not that appeared on llvm-commits archive) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051

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

dblaikie added a comment. In D117616#3732261 , @dblaikie wrote: > In D117616#3731188 , @abrachet > wrote: > >> While digging around failures we were seeing related to this I found some >> behavior that diverges

[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

[PATCH] D135916: Itanium ABI: Pack non-pod members of packed types

dblaikie created this revision. dblaikie added reviewers: rnk, abrachet. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Seems there's a narrow case - where a packed type doesn't pack its base

[PATCH] D135488: [codegen][WIP] Display stack layouts in console

dblaikie added a comment. In D135488#3854624 , @nickdesaulniers wrote: > Playing around with `llvm-dwarfdump` today, I think I found an interesting > example: > > Consider the following DWARF DIE: > > 0x00017e5e: DW_TAG_variable >

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

dblaikie added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:1569 + virtual bool areDefaultedSMFStillPOD(const LangOptions&) const; + rnk wrote: > Please add a doc comment for this, with some history about how previously > explicitly

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a comment. In D135326#3853324 , @rjmccall wrote: > In D135326#3851678 , @dblaikie > wrote: > >> In D135326#3851672 , @dblaikie >> wrote: >> >>>

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a comment. In D135326#3851672 , @dblaikie wrote: > (abandoned this, but still kind of curious) > > @rjmccall - any background/history of having the CXXABI distinct from the OS? > I guess the point might've been that the C ABI is part

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added a subscriber: rjmccall. dblaikie added a comment. (abandoned this, but still kind of curious) @rjmccall - any background/history of having the CXXABI distinct from the OS? I guess the point might've been that the C ABI is part of/the definition of the OS, but maybe the CXX ABI

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

dblaikie updated this revision to Diff 467024. dblaikie added a comment. Add doc comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/include/clang/Basic/LangOptions.h

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a subscriber: rnk. dblaikie added a comment. In D135551#3850391 , @aaron.ballman wrote: > In D135551#3850308 , @dblaikie > wrote: > >> In D135551#3850266

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. In D135551#3850266 , @inclyc wrote: > This makes sense! However I think `assert(0)` should not be used in this > case, we could expose another `llvm_unreachable`-like api and probably > `llvm_report_error` shall be fine. Are

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. >> Generally LLVM's pretty hard to fathom in a non-asserts build anyway, right? >> (that's the first thing any of us do is reproduce with an assertions build >> that may fail miles away from where a crash occurred because an invariant >> was violated much earlier) -

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3849673 , @iains wrote: > (trying not derail this discussion further) > > - Yes, the alternate solution proposed for the "Hello World" case would work > with the module mapper interface. However, the initial

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. Because I was looking, some other `assert(0)` -> `llvm_unreachable` cleanups (though, yes, even the earliest cleanups include some assert(0) -> report_fatal_error, but for externally/user-reachable failures, like invalid bitcode, I think). Some of these are more

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. To the original point I was making about implicit modules (which might've been my own confusion due to the term being brought up in D134269 ): Implicit modules is the situation where the compiler, finding that it needs a module while

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. In D135551#3847607 , @aaron.ballman wrote: > In D135551#3847444 , @dblaikie > wrote: > >> I thought this was settled quite a while ago and enshrined in the style >> guide:

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. Can't seem to find an llvm-dev/commits discussion for r166821, but I remember discussing it several times before, perhaps this one happened on IRC and so we may not have any record of it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dblaikie added a comment. I thought this was settled quite a while ago and enshrined in the style guide: https://llvm.org/docs/CodingStandards.html#assert-liberally `assert(0)` should not be used if something is reachable. We shouldn't have a "this violates an invariant, but if you don't have

[PATCH] D119209: [clang] Add cc1 option -fctor-dtor-return-this

dblaikie added a comment. I was rather hoping we could have a more general "floating ABI" flag that we can lump in anything that we'd like to have otherwise but can't break ABI for - so users like Fuschia that know they're compiling everything with the same version of Clang can opt into the

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

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 = !DefaultsToAIXPowerAlignment ?

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

dblaikie added a comment. In D134267#3844605 , @ChuanqiXu wrote: > In D134267#3815453 , @dblaikie > wrote: > >>> This also tries to fix the problem I raised a year ago: >>>

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added inline comments. Comment at: clang/lib/Basic/Targets/OSTargets.h:169 +if (T.getOS() == llvm::Triple::WatchOS || +this->getCXXABI().getKind() == TargetCXXABI::AppleARM64) + return TargetInfo::UseTailPaddingUnlessPOD11; dblaikie

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie added inline comments. Comment at: clang/lib/Basic/Targets/OSTargets.h:169 +if (T.getOS() == llvm::Triple::WatchOS || +this->getCXXABI().getKind() == TargetCXXABI::AppleARM64) + return TargetInfo::UseTailPaddingUnlessPOD11; rnk wrote: >

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

dblaikie updated this revision to Diff 465957. dblaikie added a comment. Fix to use CXXABI Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135326/new/ https://reviews.llvm.org/D135326 Files: clang/include/clang/Basic/TargetCXXABI.h

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

dblaikie added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:774-775 +if ((!Constructor->isDeleted() && !Constructor->isDefaulted()) || +(getLangOpts().getClangABICompat() <= + LangOptions::ClangABI::Ver15 || Target.isPS() ||

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

dblaikie updated this revision to Diff 465956. dblaikie added a comment. Move condition to TargetInfo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 Files: clang/include/clang/Basic/LangOptions.h

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

dblaikie added a comment. In D119051#3834985 , @rnk wrote: > Relatedly, if we put this POD behavior query on TargetInfo, I wonder if it > makes sense to move the tail padding predicate from TargetCXXABI to > TargetInfo: >

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