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

[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

[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

[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

[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

[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

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

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

[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

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

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

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

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

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

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

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

[PATCH] D112280: Support: Use Expected::moveInto() in a few places

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. These look good (pending the underlying change). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112280/new/ https://reviews.llvm.org/D112280

[PATCH] D112253: [test] Make sure plugin actually runs in clear-ast-before-backend-plugins.c

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

[PATCH] D112190: [clang] Don't clear AST if we have consumers running after the main action

dblaikie added inline comments. Comment at: clang/test/Misc/clear-ast-before-backend-plugins.c:3-8 +// RUN: %clang_cc1 -mllvm -debug-only=codegenaction -clear-ast-before-backend -emit-obj -o /dev/null -load %llvmshlibdir/PrintFunctionNames%pluginext %s 2>&1 | FileCheck %s

[PATCH] D112190: [clang] Don't clear AST if we have consumers running after the main action

dblaikie accepted this revision. dblaikie added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/Misc/clear-ast-before-backend-plugins.c:3-8 +// RUN: %clang_cc1 -mllvm -debug-only=codegenaction -clear-ast-before-backend -emit-obj -o

[PATCH] D110287: [modules] While merging ObjCInterfaceDecl definitions, merge them as decl contexts too.

dblaikie added a comment. In D110287#3074175 , @vsapsai wrote: > In D110287#3074082 , @dblaikie > wrote: > >> In D110287#3073804 , @vsapsai >> wrote: >> >>> Pre-merge

[PATCH] D110287: [modules] While merging ObjCInterfaceDecl definitions, merge them as decl contexts too.

dblaikie added a comment. In D110287#3073804 , @vsapsai wrote: > Pre-merge checks are passing now after the rebase. I believe it is a > straightforward change and we are merging decl contexts for other Decls > already, so merging them for

[PATCH] D112100: [clang] Add option to disable -clear-ast-before-backend

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good - possible flag def improvement with `BoolOption`. Comment at: clang/include/clang/Driver/Options.td:5317-5319 +def no_clear_ast_before_backend : Flag<["-"],

[PATCH] D112096: [clang] Add plugin ActionType to run command line plugin before main action

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

[PATCH] D111184: [clang] Allow printing 64 bit ints in diagnostics

dblaikie added a comment. Is there no diagnostic that could reasonably produce such a large value that would be easy to test (without needing a giant binary, etc)? (seem surprising to see this change without any test coverage) Like a 64 bit non-type template parameter or the like, perhaps?

[PATCH] D111973: [clang] Disable -clear-ast-before-backend with -print-stats

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Could you check some of the stats output is valid/expected, not just corrupt/garbage? "just doesn't crash" isn't a great criteria for a test. Repository: rG LLVM Github Monorepo

[PATCH] D111270: [clang] Pass -clear-ast-before-backend in Clang::ConstructJob()

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D111270#3062802 , @aeubanks wrote: > In D111270#3060484 , @dblaikie > wrote: > >> Plan is still to

[PATCH] D111767: [clang] Support -clear-ast-before-backend without -disable-free

dblaikie accepted this revision. dblaikie added subscribers: aaron.ballman, rsmith. dblaikie added a comment. This revision is now accepted and ready to land. Sounds OK, thanks! Comment at: clang/lib/CodeGen/CodeGenAction.cpp:355-356 + if

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

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

[PATCH] D111767: [clang] Support -clear-ast-before-backend without -disable-free

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:355-356 + if (CodeGenOpts.ClearASTBeforeBackend) { +// The ASTContext may be unusable after this. +C.cleanup(); C.getAllocator().Reset(); Any chance of

[PATCH] D111270: [clang] Pass -clear-ast-before-backend in Clang::ConstructJob()

dblaikie added a comment. Plan is still to address the "this only works with disable free" issue? (I've some preference that be addressed before this feature is turned on by default) & is there a flag to pass to turn this off if someone had trouble with it? (doesn't necessarily have to be, but

[PATCH] D111582: [clang] Teardown new PM data structures before running codegen pipeline

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks great! Comment at: clang/lib/CodeGen/BackendUtil.cpp:1441-1442 - - // FIXME: We still use the legacy pass manager to do code generation. We - // create that pass

[PATCH] D111582: [clang] Clear IR analyses before codegen pipeline

dblaikie added a comment. Any chance of refactoring this code so naturally these objects are lexically scoped to their necessary lifetime, and just cease to exist before the code gen passes are run? (looks like this function is pretty long and might benefit from being broken up into pieces

[PATCH] D111521: [DebugInfo] Mark OpenMP generated functions as artificial

dblaikie added a comment. Might not hurt to have some negative test cases too - like something that is a VarDecl but has NoStub DynamicInitKind? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111521/new/ https://reviews.llvm.org/D111521

[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); dblaikie wrote: > Looks like this (&

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

dblaikie created this revision. dblaikie added reviewers: aprantl, probinson, JDevlieghere. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. There's a nuanced check about when to use suffixes on these integer

[PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf6a561c4d675: DebugInfo: Use clangs preferred names for integer types (authored by dblaikie). Herald added subscribers: llvm-commits, lldb-commits,

[PATCH] D111105: [clang] Add option to clear AST memory before running LLVM passes

dblaikie added a comment. In D05#3046631 , @aeubanks wrote: > In D05#3046585 , @dblaikie > wrote: > >>> This is similar to perf testing which we don't really have in tree tests >>> for. Typically these

[PATCH] D111105: [clang] Add option to clear AST memory before running LLVM passes

dblaikie added a comment. > This is similar to perf testing which we don't really have in tree tests for. > Typically these things are mostly monitored separately (e.g. > llvm-compile-time-tracker). Except in this case it isn't tested at all because it's behind a flag. Unless we're bringing

[PATCH] D111105: [clang] Add option to clear AST memory before running LLVM passes

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D05#3043734 , @aeubanks wrote: > update test > I checked to make sure that we're not accepting -clear-ast-before-backend as > a driver

[PATCH] D111105: [clang] Add option to clear AST memory before running LLVM passes

dblaikie requested changes to this revision. dblaikie added a subscriber: aaron.ballman. dblaikie added a comment. This revision now requires changes to proceed. Oh, looking at this ab it further I do have some things to discuss. Comment at:

[PATCH] D111105: [clang] Add option to clear AST memory before running LLVM passes

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems like a good place to start with experimentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D05/new/

[PATCH] D110665: [clang] Don't use the AST to display backend diagnostics

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

[PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

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

[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); Looks like this (& the

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

dblaikie added a comment. In D77598#3035591 , @v.g.vassilev wrote: > In D77598#3035449 , @dblaikie wrote: > >> Came across this while trying to do "simplified template names" - producing >> template names in

[PATCH] D110891: [inliner] Mandatory inlining decisions produce remarks

dblaikie added inline comments. Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:52 +namespace { +using namespace llvm::ore; mtrofin wrote: > wenlei wrote: > > mtrofin wrote: > > > wenlei wrote: > > > > curious why do we need anonymous namespace here? > > >

[PATCH] D110278: Adding doWithCleanup abstraction

dblaikie added inline comments. Comment at: clang/lib/Frontend/FrontendAction.cpp:553-565 + bool HasBegunSourceFile = false; + std::function Operation = [&]() { +return BeginSourceFileImpl(CI, RealInput, HasBegunSourceFile); + }; + std::function Cleanup = [&]() { +if

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

dblaikie added a comment. Came across this while trying to do "simplified template names" - producing template names in DWARF without template parameter lists as part of the textual name, then rebuilding that name from the structural representation of template parameters in DWARF

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

dblaikie created this revision. dblaikie added a reviewer: rsmith. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Improve the application of D77598 by passing the template parameter list,

[PATCH] D110665: [clang] Don't use the AST to display backend diagnostics

dblaikie added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:336-337 + for (auto : getModule()->functions()) { +if (const Decl *FD = Gen->GetDeclForMangledName(F.getName())) { + auto Loc =

[PATCH] D110673: [clang] Don't modify OptRemark if the argument is not relevant

dblaikie added a comment. It'd be good to understand/document (maybe document in the form of a test if possible) how downstream users are relying on this - perhaps it's not a valid reliance and we shouldn't maintain compatibility? Maybe it is and we should ensure some test coverage of the sort

[PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

dblaikie created this revision. dblaikie added reviewers: probinson, aprantl. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This reverts c7f16ab3e3f27d944db72908c9c1b1b7366f5515 / r109694 - which suggested this was done to

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

dblaikie added a comment. There have been a few different patches going around related to mis-scoped things - imported entities and types, maybe other things too? (static variables? Or has that already been fixed now) Perhaps some summary of the state of all these patches sent to

[PATCH] D109865: [NFC] `goto fail` has failed us in the past...

dblaikie added a comment. In D109865#3014106 , @beanz wrote: > I was talking with @lhames the other day about building a `doWithCleanup` > abstraction that could take a labmda to perform and a lambda to cleanup on > failure. > > I was thinking I may

[PATCH] D109865: [NFC] `goto fail` has failed us in the past...

dblaikie added a comment. (A possibly more robust option - so that this doesn't regress if someone adds a new "return true" codepath & forgets to release the cleanup (it looks like a pretty long function with several exits, etc) - would be to move the code into an "impl" function and have the

[PATCH] D110204: Make DiagnosticInfoResourceLimit's limit param required

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

[PATCH] D110201: [clang] Make -Rpass imply -Rpass=.*

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

[PATCH] D110127: [Clang] Support typedef with btf_tag attributes

dblaikie added a comment. Given this is about being preserved into debug info - I imagine it'll have the same behavior as using a typedef in a function return type - whenever that currently shows up in the DWARF, this attribute would. Where it doesn't, this doesn't. So I wouldn't expect this

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie added inline comments. Comment at: clang/lib/AST/Type.cpp:3045 case NullPtr: -return "nullptr_t"; +return "std::nullptr_t"; case Overload: aaron.ballman wrote: > dblaikie wrote: > > aaron.ballman wrote: > > > Should this be

[PATCH] D110044: Print nullptr_t namespace qualified within std::

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG131e8786640a: Print nullptr_t namespace qualified within std:: (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie updated this revision to Diff 373768. dblaikie added a comment. Add debug info test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110044/new/ https://reviews.llvm.org/D110044 Files: clang/lib/AST/Type.cpp

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie added a comment. In D110044#3009201 , @aaron.ballman wrote: >> This improves diagnostic (& important to me, DWARF) accuracy > > FWIW, I don't think the diagnostic particularly needs more accuracy here -- I > think users know what `nullptr_t`

[PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. Thanks for the suggestions/details, @dexonsmith - I've posted to llvm-dev here: https://groups.google.com/g/llvm-dev/c/m9UVRhzJvh4/m/qdd_SyPuCQAJ and will wait for some follow-up (or dead silence) before starting along probably your latter suggestion. Repository:

[PATCH] D110044: Print nullptr_t namespace qualified within std::

dblaikie created this revision. dblaikie added reviewers: aaron.ballman, rtrieu. dblaikie requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. This improves diagnostic (& important to me, DWARF)

[PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. In D109345#2990577 , @dexonsmith wrote: > In D109345#2990527 , @dblaikie > wrote: > >> (were there other regressions I mentioned/should think about?) > > I don't have specific

[PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. In D109345#2987565 , @dexonsmith wrote: > This seems like the right direction to me! Especially like the > look-through-the-ErrorInfo change for `FileError` -- I hit this before and > found it annoying. Thanks for taking a

[PATCH] D109411: [clang] Enable the special enable_if_t diagnostics for libc++'s __enable_if_t as well.

dblaikie added a comment. Looks like most of the testing for the enable_if custom dialog was added in `clang/test/SemaTemplate/overload-candidates.cpp` in rG6f8d2c6c9c3451effdf075a7034bbe77045bfeba . (improved in

[PATCH] D109411: [clang] Enable the special enable_if_t diagnostics for libc++'s _EnableIf as well.

dblaikie added a comment. Would at. least need a test case, but otherwise sounds OK to make things a bit better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109411/new/ https://reviews.llvm.org/D109411

[PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. In D109345#2986297 , @thopre wrote: > Is there no way to split this patch further? It's going to be hard finding > someone who can review something so big. If there's no way to split it in > incremental changes, you could

[PATCH] D108756: [clang] Add '-ast-dump-filter=' support

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

[PATCH] D108794: Fully qualify template template parameters when printing

This revision was automatically updated to reflect the committed changes. dblaikie marked an inline comment as done. Closed by commit rG5fb3f43778f8: Fully qualify template template parameters when printing (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D108794: Fully qualify template template parameters when printing

dblaikie marked an inline comment as done. dblaikie added a comment. Ping In D108794#2976063 , @rtrieu wrote: > It looks like a strict improvement on printing and most callers using the > default args won't need to be updated. > > There's one more

[PATCH] D108794: Fully qualify template template parameters when printing

dblaikie updated this revision to Diff 370086. dblaikie added a comment. Herald added subscribers: usaxena95, kadircet. Herald added a project: clang-tools-extra. Fix the clang-tools-extra caller, and update the TemplateName::print doc comment Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. @rsmith - would it make sense to disable preferred names use when `PrintingPolicy::PrintCanonicalTypes` is used? It's used in `CGDebugInfo`, but also in `Sema::findFailedBooleanCondition` - so maybe we need another `PrintingPolicy` flag for this so `CGDebugInfo` can

[PATCH] D108794: Fully qualify template template parameters when printing

dblaikie created this revision. dblaikie added reviewers: aaron.ballman, rtrieu. Herald added a subscriber: arphaman. dblaikie requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. I discovered this quirk when working on some DWARF - AST

[PATCH] D106618: [Clang][LLVM] generate btf_tag annotations for DISubprogram types

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Eh, sure, don't mind about the test either way (the other test cases are optimization agnostic, I think - so they could all be lumped in together - there might also be a flag to turn on

[PATCH] D106618: [Clang][LLVM] generate btf_tag annotations for DISubprogram types

dblaikie added inline comments. Comment at: clang/test/CodeGen/attr-btf_tag-disubprogram-2.c:5-14 +#define __tag2 __attribute__((btf_tag("tag2"))) + +struct t1 { + int a; +}; + +extern int __tag1 __tag2 foo(struct t1 *); Any particular reason this needs to be

[PATCH] D106618: [Clang][LLVM] generate btf_tag annotations for DISubprogram types

dblaikie added a comment. The clang side might benefit from a test with a function declaration, a call to that function, but no definition of it - with optimizations enabled, clang should emit the DISubprogram for that function for the purpose of call site DWARF, I think? (or maybe that

[PATCH] D106620: [Clang][LLVM] generate btf_tag annotations for func parameters

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

[PATCH] D106619: [Clang][LLVM] generate btf_tag annotations for DIGlobalVariable

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good. (any chance have you measured/could you measure how much larger the bitcode for, say, an LLVM self-host Debug build gets with all these btf patches? (even without

[PATCH] D106616: [Clang][LLVM] generate btf_tag annotations for DIDerived types

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good. Please do the `hasAttr`/`CollectBTFTagAnnotations` refactor in a separate preliminary commit (cleaning up any existing callers that have the null check like that), then

[PATCH] D108407: [CodeGen][WIP] Avoid generating Record layouts for pointee types

dblaikie added a comment. Notion seems plausible - though if there's some way to refactor so there's less need for manual insertion/maintenance of calls to `ConvertTypeForMem` that'd be good/important. I don't think there'd be anything fundamentally wrong with this approach - though checking

[PATCH] D106615: [Clang][LLVM] generate btf_tag annotations for DIComposite types

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks alright - please commit the LLVM and Clang portions of this separately. (LLVM first, shouldn't require any changes to clang/should be standalone, then committing the clang change

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

dblaikie added a comment. The current state of the root `.clang-tidy` accurately reflects the LLVM Coding Conventions as documented, which applies to LLVM and its subprojects (some subprojects have diverged from this standard). The place for a discussion to change the naming convention is not

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

dblaikie added a comment. In D108265#2952555 , @MaskRay wrote: > The number of top-level projects using `VariableName` is smaller than the > number of projects not using the style. > The top-level variable style just provoked projects to either override

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

dblaikie added a comment. (eg: new projects should benefit from the LLVM default- less chance of further diversification of naming conventions) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108265/new/ https://reviews.llvm.org/D108265

[PATCH] D108265: .clang-tidy: Push variable related readability-identifier-naming options down to projects

dblaikie added a comment. I think it should probably stay as-is, given this is the documented LLVM project naming convention: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly - a bit of extra friction for subprojects to opt-out of that

[PATCH] D106615: [Clang][LLVM] generate btf_tag annotations for DIComposite types

dblaikie added a comment. In D106615#2950964 , @yonghong-song wrote: > The attributes should be revolved during semantic analysis stage. So looks > like replace-style attribute setting is not really needed. I will change to > add attribute parameter

[PATCH] D106615: [Clang][LLVM] generate btf_tag annotations for DIComposite types

dblaikie added a comment. Can these values/this attribute list vary over the course of a source file? (eg: the first use of a type might only have one of the attributes specified at that point, but then more attributes appear later - and then those attributes should be applied too?) Perhaps a

[PATCH] D106615: [Clang][LLVM] generate btf_tag annotations for DIComposite types

dblaikie added a comment. In D106615#2949685 , @yonghong-song wrote: > @dblaikie ping. Could you help take a look at the patch? Yeah, it's on my list. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3478-3479 + if (D->hasAttr()) +

[PATCH] D107933: [clang] Expose unreachable fallthrough annotation warning

dblaikie added a comment. Patch description probably needs an update - looks like it's out of date ("Add -Wimplicit-fallthrough-unreachable, which is default enabled with -Wimplicit-fallthrough" should read, I guess "Add -Wunreachable-code-fallthrough, which is default enabled with

[PATCH] D107933: [clang] Expose unreachable fallthrough annotation warning

dblaikie added a comment. In D107933#2944204 , @aaron.ballman wrote: > In D107933#2944135 , @nathanchance > wrote: > >> In D107933#2942430 , @xbolva00 >> wrote: >> >>>

[PATCH] D107933: [clang] Expose unreachable fallthrough annotation warning

dblaikie added a comment. Probably still worth fixing the bug too? (though maybe not a priority once it's moved out into -Wunreachable-code) - though the general fix, as discussed on the bug (comment 18 and 19 about why this doesn't already produce an unreachable-code warning), may require a

[PATCH] D107933: [clang] Expose unreachable fallthrough annotation warning

dblaikie added a comment. In D107933#2942023 , @xbolva00 wrote: > GCC does not warn (with common -Wall) for this case, right? I think Clang > should not as well. > > ImplicitFallthroughUnreachable could be enabled with -Wunreachable-code, if > you

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. Oh, I should say I wasn't able to get this behavior to be exposed in a diagnostic in my limited testing - and attempting to add diagnostic cases caused me to lose the reproduction in the ast dumping, perhaps because I added some AST that tickled/fixed the disparity

[PATCH] D91311: Add new 'preferred_name' attribute.

dblaikie added a comment. While looking into some debug info issues* I've come across what I think are some inconsistencies in the handling of this attribute - @rsmith mind taking a look? Firstly, it looks like something isn't handled when it comes to templates where only the declaration of a

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

dblaikie added a comment. In D106585#2905916 , @rnk wrote: > In D106585#2905663 , @dblaikie > wrote: > >> In D106585#2902588 , @dblaikie >> wrote: >> >>> Preserving

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