[PATCH] D105014: added some example code for llvm::Expected

dblaikie added a comment. In D105014#2873739 , @sammccall wrote: > In D105014#2872674 , @dblaikie > wrote: > >> +@lhames for context as the author/owner of `llvm::Error` and associated >> things. >> >> Perhaps i

[PATCH] D105457: [clang] Refactor AST printing tests to share more infrastructure

dblaikie added a comment. In D105457#2873838 , @glaubitz wrote: > In D105457#2873294 , @dyung wrote: > >> This change I suspect is causing a build failure on a build bot. >> >> https://lab.llvm.org/buildbot/#/buil

[PATCH] D102728: [clang][Sema] removes -Wfree-nonheap-object reference param false positive

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Could probably haggle over the tests some more, but might be diminishing returns at this point. Looks good! Might suggest separating the code cleanup mlir changes from the warning change

[PATCH] D105457: [clang] Refactor AST printing tests to share more infrastructure

dblaikie added a comment. In D105457#2874783 , @nridge wrote: > To be honest, I don't really understand this error. It seems to come from a > compile step (not a link step), and it comes from the assembler and says > "symbol ... is already defined"? I d

[PATCH] D105457: [clang] Refactor AST printing tests to share more infrastructure

dblaikie added a comment. In D105457#2876511 , @dyung wrote: > If it helps, I have so far been able to reduce the file to this which still > shows the failure when compiled with gcc 7.5: Could you provide the preprocessed input file - maybe that run thr

[PATCH] D112519: parallel-libs: remove some artifacts

dblaikie added inline comments. Comment at: clang/docs/ClangFormattedStatus.rst:7807-7821 - * - parallel-libs/acxxel - - `6` - - `4` - - `2` - - :part:`66%` - * - parallel-libs/acxxel/examples - - `1` I didn't touch this because I think it

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie added a comment. In D99#3093448 , @aaron.ballman wrote: > Attribute bits look good to me, but I'd appreciate if @dblaikie could weigh > in on whether he thinks the CodeGen changes are fine. My concern there is > around whether the changes

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie added a comment. In D99#3096124 , @aaron.ballman wrote: > In D99#3095623 , @yonghong-song > wrote: > >>> Ah, yeah, I see what you mean - that does seem sort of unfortunate. Is it >>> possible t

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie added a comment. In D99#3099988 , @aaron.ballman wrote: > In D99#3097692 , @dblaikie > wrote: > >> In D99#3096124 , >> @aaron.ballman wrote: >> >>>

[PATCH] D112628: NOT READY FOR REVIEW Nothing to see here, just exporting to show this to someone.

dblaikie added a comment. Rather than sending a review to the lists that you don't need review for, you can create draft reviews that send no mail and share them manually with your intended audience, discussed/documented here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-c

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

dblaikie added a comment. In D107049#3100630 , @rnk wrote: > So, to back up a bit, do I understand correctly that this change adds tests > to the check-clang test suite that JIT compiles C++ code for the host and > throws C++ exceptions? Can we reconsid

[PATCH] D111477: DO NOT SUBMIT: workaround for context-sensitive use of non-type-template-parameter integer suffixes

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8bf12445383b: DebugInfo: workaround for context-sensitive use of non-type-template-parameter… (authored by dblaikie). Herald added a project: libc++.

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

dblaikie added a comment. In D107049#3103984 , @lhames wrote: > In D107049#3101456 , @dblaikie > wrote: > >> In D107049#3100630 , @rnk wrote: >> >>> So, to back up a bit,

[PATCH] D111477: DO NOT SUBMIT: workaround for context-sensitive use of non-type-template-parameter integer suffixes

dblaikie added a comment. In D111477#3106171 , @ldionne wrote: > This broke libc++'s CI: > https://buildkite.com/llvm-project/libcxx-ci/builds/6374#7569da95-c852-44f9-8b69-947245cf0b65 > > When you make a change to Clang AND the libc++ tests at the same

[PATCH] D111199: [Clang][LLVM][Attr] support btf_type_tag attribute

dblaikie accepted this revision. dblaikie added a comment. Seems unfortunate that attributes on types are only available through TypeLocs rather than through sugar (like if we used a typedef it'd be visible in the type sugar, but not if it's written on the type usage itself) - but that's above

[PATCH] D113352: [clang] Run LLVM Verifier in modes without CodeGen too

dblaikie added a comment. I guess CodeGenOpts::VerifyModule is on by default, but I'll admit that surprises me - for API-built modules, I figured it was expected they were valid by construction (ie: like an assertion - it's a bug in clang if the resulting module isn't valid - so we wouldn't wan

[PATCH] D112971: [NFC] Remove LinkAll*.h

dblaikie added a comment. My understanding is that this is necessary for the legacy pass manager that uses a global registration system - if you didn't reference any function in the pass, then the code wouldn't get linked in - because the only way the pass was accessed was through the registry?

[PATCH] D112971: [NFC] Remove LinkAll*.h

dblaikie added a comment. In D112971#3117441 , @dblaikie wrote: > My understanding is that this is necessary for the legacy pass manager that > uses a global registration system - if you didn't reference any function in > the pass, then the code wouldn'

[PATCH] D113391: [Modules] Don't support clang module and c++20 module at the same time

dblaikie added inline comments. Comment at: clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json:3 + "directory" : "DIR", + "command" : "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 -I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fmodules-cache-path=DIR/mo

[PATCH] D110898: Pass template parameters when printing template argument lists for function templates

dblaikie abandoned this revision. dblaikie added a comment. Talked to Richard offline - this is undesirable because it would mean that certain template overloads would not be disambiguated in their naming. I'll post a patch with a test case that demonstrates that desired behavior and abandon th

[PATCH] D111477: DO NOT SUBMIT: workaround for context-sensitive use of non-type-template-parameter integer suffixes

dblaikie added a subscriber: rsmith. dblaikie added a comment. Some offline post-commit feedback from @rsmith - Rename printing policy to match closer to the function it's tested in. Will think about that, check the function name, etc, and commit a patch in the next few days, hopefully. Reposi

[PATCH] D111477: DO NOT SUBMIT: workaround for context-sensitive use of non-type-template-parameter integer suffixes

dblaikie added a comment. Renamed in 6512098877c3a230bbd38bc81b14a4e5844739ff to `AlwaysIncludeTypeForTemplateArgument` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111477/new/

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

dblaikie added a comment. In D97411#2591030 , @aaron.ballman wrote: > Thanks! I am still curious about the forward declare/redeclaration behavior > and whether that is a situation that makes sense or not. I suspect this case > may make sense (and likely

[PATCH] D97706: Make -f[no-]split-dwarf-inlining CC1 default align with driver default (no inlining)

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds alright - I guess this helps reduce -cc1 command lines when they contain the more common/default value? (might be worth mentioning that in the commit to explain the motivation for t

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

dblaikie added a comment. In D97411#2595049 , @akhuang wrote: > In D97411#2594345 , @ldionne wrote: > >> I don't have an opinion about the attribute itself. I do have an opinion >> about using that attribute in lib

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

dblaikie added a comment. In D97411#2598625 , @akhuang wrote: > In D97411#2595155 , @dblaikie wrote: > >> In D97411#2595049 , @akhuang wrote: >> >>> In D97411#2594345

[PATCH] D98146: OpaquePtr: Turn inalloca into a type attribute

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. > I think byval/sret and the others are close to being able to rip out the code > to support the missing type case. A lot of this code is shared with inalloca, > so catch this up to the ot

[PATCH] D98146: OpaquePtr: Turn inalloca into a type attribute

dblaikie added inline comments. Comment at: llvm/lib/IR/Attributes.cpp:490 if (Type *Ty = getValueAsType()) { - raw_string_ostream OS(Result); + // FIXME: This should never be null Result += '('; arsenm wrote: > dblaikie wrote: > > Is it? Co

[PATCH] D95244: [clang][AST] Handle overload callee type in CallExpr::getCallReturnType.

dblaikie added a comment. Added a couple of other folks who might have more context on the tooling front... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95244/new/ https://reviews.llvm.org/D95244 ___ c

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me! Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2388-2390 + if (DebugKind > codegenoptions::LimitedDebugInfo || + RD->hasAttr()) return false;

[PATCH] D77598: Integral template argument suffix and cast printing

dblaikie added inline comments. Comment at: clang/lib/AST/DeclPrinter.cpp:1101 Out << ", "; -Args[I].print(Policy, Out); +if (TemplOverloaded || !Params) + Args[I].print(Policy, Out, /*IncludeType*/ true); rsmith wrote: > dblaikie wrote: > > d

[PATCH] D113743: [RFC][clang][DebugInfo] Allow function-local statics and types to be scoped within a lexical block

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Guessing Adrian probably meant to sign off on this, judging by his phrasing but maybe forgot to hit the "accept" button. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[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, thou

[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] 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] 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 https:/

[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 https://reviews.

[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 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 hom

[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 va

[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 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 than

[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] 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 at

[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] 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] 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] 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] 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] 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 fully

[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 quickly;

[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 wro

[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 in

[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 t

[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 behavi

[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 a

[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 have

[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 somethin

[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, I

[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 me

[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 intermedi

[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 oth

[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". Differ

[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] 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 C

[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 muc

[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] 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. > 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 thin

[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 m

[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: https://reviews.l

[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: https://reviews.llv

[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] 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 e4ec6ce8a75c208b49b163c81cda9

[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] 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 = Struct1::enumVal

[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 change

[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 DW

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

dblaikie added inline comments. Comment at: llvm/include/llvm/ADT/BreadthFirstIterator.h:128-131 + bool operator==(iterator_sentinel) const { return VisitQueue.empty(); } + + bool operator!=(iterator_sentinel RHS) const { return !(*this == RHS); } + rusyaev-rom

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

dblaikie added inline comments. Comment at: clang/docs/CPlusPlus20Modules.rst:395-396 + +Roughly, this theory is correct. But the problem is that it is too rough. Let's see what actually happens. +For example, the behavior also depends on the optimization level, as we will illu

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

dblaikie added a comment. >> There could be more testing than only the indirect result of the packing >> problem that first inspired this patch. Any suggestions on what might be the >> most direct way to test whether the type's been considered pod in this sense? > > I would have thought use of

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

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

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

dblaikie updated this revision to Diff 452023. dblaikie added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118511/new/ https://reviews.llvm.org/D118511 Files: clang/include/clang/Basic/DiagnosticASTKinds.td clang/include/cla

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

dblaikie added a comment. I guess the other packed behavior/ABI checking 277123376ce08c98b07c154bf83e4092a5d4d3c6 In D119051#3715939 , @aaron.ballman wrote: > In D119051#3714645

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

dblaikie added a comment. > So I could test this other ways that actually impact layout - like whether > things can be packed in tail padding (can pack in tail padding for non-pod, > right?). Or we could ast dump and inspect the property directly? Maybe there > are some other options? Here's w

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

dblaikie added a comment. I guess the other way to test for pod-for-purposes-of-ABI is IRgen. Looks like MSVC isn't observable based on sizeof or alignof on these issues (the previous godbolt shows MSVC's answer to alignment for the packed and size for the trailing packing don't change based on

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

dblaikie added a comment. Actually, when it comes to diagrams - maybe what'd be good is a diagram of classic compilation and a diagram of modules and header modules src1.cpp -+> clang++ src1.cpp --> src1.o ---, hdr1.h --' +-> clang++ src1.o src2.o -> execu

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

dblaikie updated this revision to Diff 452315. dblaikie added a comment. Add PS4/Darwin handling Remove unnecessary warning suppression Add MSVC return ABI testing Add tail-padding based testing as well as the alignment based testing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

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

dblaikie added a subscriber: hansw. dblaikie added a comment. In D119051#3717934 , @dblaikie wrote: > I guess the other way to test for pod-for-purposes-of-ABI is IRgen. Looks > like MSVC isn't observable based on sizeof or alignof on these issues (the

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

dblaikie marked an inline comment as done and an inline comment as not done. dblaikie added inline comments. Comment at: clang/test/SemaCXX/class-layout.cpp:2-9 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++

[PATCH] D131933: DebugInfo: Remove auto return type representation support

dblaikie created this revision. dblaikie added reviewers: aprantl, probinson, shafik, awpandey. Herald added a project: All. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Seems this complicated lldb sufficiently for some cases th

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

dblaikie added a comment. In D123319#3723648 , @Michael137 wrote: > FYI, had an offline chat with @dblaikie and we decided that the best way > forward would be to try stop emitting auto return types in DWARF, instead of > relying on lambda/member-funct

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

dblaikie added a comment. Herald added a project: All. I've posted D131933 to revert this patch based on discussion in D123319 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70524

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added a comment. Probably easier to review if it's framed somewhat more like @MaskRay and my examples - backwards compatible, without fixing all the API users as we can discuss those separately. Though it is useful to have one or two example uses - but having all the API uses being up

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added a comment. (still a fair few unhandled comments from the last round - guess work to address those is still ongoing) Comment at: llvm/include/llvm/Support/Compression.h:95-98 + static CompressionSpecRef Unknown; + static CompressionSpecRef None; + static Compr

[PATCH] D131933: DebugInfo: Remove auto return type representation support

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG06c70e9b998c: DebugInfo: Remove auto return type representation support (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE

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

dblaikie added a comment. In D117616#3732173 , @tstellar wrote: > @dblaikie Is there anything we need to do in the release branch for this > still? cc @thieta Sorry, yeah, I'd tagged you over here: https://reviews.llvm.org/D118511#3717753 for part of t

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

dblaikie added a comment. In D117616#3731188 , @abrachet wrote: > While digging around failures we were seeing related to this I found some > behavior that diverges from gcc. See https://godbolt.org/z/qe18Wh4jf thanks for the bug report, yeah - looks li

[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

dblaikie added inline comments. Comment at: llvm/include/llvm/Object/Decompressor.h:52-53 uint64_t DecompressedSize; + compression::CompressionSpecRef CompressionScheme = + compression::CompressionSpecRefs::Zlib; }; ckissane wrote: > dblaikie wrote: > >

[PATCH] D132232: Update coding standards for constexpr if statements; NFC

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Yeah, I'm OK with this, though yeah, having an example where the `else` is necessary, but I don't mind too much. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

<    4   5   6   7   8   9   10   11   12   13   >