[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 `TemplOverload

[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 https://lists.llvm.org/cg

[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] D96597: [DebugInfo] Keep the DWARF64 flag in the module metadata

dblaikie accepted this revision. dblaikie added a subscriber: aprantl. dblaikie added a comment. This revision is now accepted and ready to land. @aprantl - mind having a look at this/signing off as well, if you're OK with it? Since it's a core metadata change, wouldn't mind some extra eyes on it

[PATCH] D96783: [Driver] Support -gdwarf64 for assembly files

dblaikie accepted this revision. dblaikie added a comment. Looks good to me - thanks Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3757-3771 + if (DwarfFormatArg && + DwarfFormatArg->getOption().matches(options::OPT_gdwarf64)) { +if (DwarfVersion < 3) + D.Dia

[PATCH] D96244: [clangd] Introduce Modules

dblaikie added a comment. "modules" is a pretty overloaded term in Clang (between llvm Modules and Clang Modules and now C++ standard Modules) - any chance of some other term that might be less overloaded in this space? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D96354: Avoid conflicts between debug-info and pseudo-probe profiling

dblaikie added a comment. @hoy Could you explain a bit further why these two features are incompatible/what the crash looks like? At first glance I wouldn't expect any debug info mode to be incompatible with any non-debug-info mode (maybe less useful, but not crashy levels of incompatible). R

[PATCH] D96597: [DebugInfo] Keep the DWARF64 flag in the module metadata

dblaikie added inline comments. Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:396 + bool Dwarf64 = + (Asm->TM.Options.MCOptions.Dwarf64 || MMI->getModule()->isDwarf64()) && + DwarfVersion >= 3 && // DWARF64 was introduced in DWARFv3. ikudrin

[PATCH] D96354: Avoid conflicts between debug-info and pseudo-probe profiling

dblaikie added a comment. In D96354#2566502 , @hoy wrote: > In D96354#2554129 , @probinson wrote: > >>> the driver had a redundant pass-through of the option >> >> I could've sworn it worked to remove that, but it d

[PATCH] D95746: clang: Exclude efi_main from -Wmissing-prototypes

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me - maybe the test could be rolled into whatever file is already testing -Wmissing-prototype and the existing special case for "main"? Do you need me to commit this, or can

[PATCH] D95746: clang: Exclude efi_main from -Wmissing-prototypes

dblaikie accepted this revision. dblaikie added a comment. Reaffirming approval with the test change - looks great! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95746/new/ https://reviews.llvm.org/D95746 __

[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. Generally OK with this, as it seemed the libc++ issue might be a bit thorny to solve. Though I'd like to hear from libc++ maintainers about how they feel to ensure they're comfortable adding this attribute in the interim (or if this might be a motivation to fix libc++

[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#2586055 , @rnk wrote: > I expected nodebug already applied to types, even though the documentation > says it only affects variables and functions. We should update the docs. Now that I think about it more, the only thi

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

dblaikie added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1665 + let Spellings = [Clang<"force_debug_if_required_type">]; + let Subjects = SubjectList<[CXXRecord]>; + let Documentation = [Undocumented]; aaron.ballman wrote: > Does this attr

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

dblaikie added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1665 + let Spellings = [Clang<"force_debug_if_required_type">]; + let Subjects = SubjectList<[CXXRecord]>; + let Documentation = [Undocumented]; akhuang wrote: > dblaikie wrote: > >

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

dblaikie added inline comments. Comment at: clang/test/CodeGenCXX/force-debug-attribute.cpp:4-12 +#if SETATTR +#define DEBUGASNEEDED __attribute__((standalone_debug)) +#else +#define DEBUGASNEEDED +#endif + +// Struct that isn't constructed, so its full type info should be omitte

[PATCH] D103667: [WIP] BPF: add support for DWARF_AT_LLVM_annotations attribute

dblaikie added subscribers: JDevlieghere, probinson, aprantl. dblaikie added a comment. Is the C source syntax already fixed? Could it use a more specific name - then maybe wouldn't need the flag to opt-in if the only places the attribute appears is where it should be propagated? This'll end up

[PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

dblaikie created this revision. dblaikie added reviewers: hokein, aaron.ballman, jyknight, mehdi_amini, kuhnel. Herald added subscribers: dcaballe, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, antiagain

[PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

This revision was automatically updated to reflect the committed changes. Closed by commit rGc5d56fec502f: NFC: .clang-tidy: Inherit configs from parents to improve maintainability (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. Any idea if the GDB test suite covers this functionality? I'd hope so, but maybe it doesn't. But yeah, at the moment I don't have any great reason to avoid the imported declaration form - so happy to go with that. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D103888: [ADT] Remove APInt/APSInt toString() std::string variants

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds OK. I wouldn't mind the places that can use op<< to use that - not sure preserving the explicit radix argument is super high value. (I think people would generally assume that's th

[PATCH] D104082: [CodeGen] Don't create a fake FunctionDecl when generating block/block_byref copy/dispose helper functions

dblaikie added a comment. In D104082#2812088 , @ahatanak wrote: > Also, is it okay to emit the `linkageName`, but not the `name` for the helper > functions? It seems okay to me, but I'm not sure. Not 100% sure - perhaps @aprantl can weigh in on this. If

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. In D103131#2811969 , @kamleshbhalui wrote: > In D103131#2811220 , @dblaikie > wrote: > >> Any idea if the GDB test suite covers this functionality? I'd hope so, but >> maybe it doesn't

[PATCH] D103888: [ADT] Remove APInt/APSInt toString() std::string variants

dblaikie added a comment. In D103888#2813236 , @RKSimon wrote: > In D103888#2811305 , @dblaikie > wrote: > >> Sounds OK. >> >> I wouldn't mind the places that can use op<< to use that - not sure >> preserving th

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. In D103131#2814595 , @kamleshbhalui wrote: > In D103131#2814015 , @dblaikie > wrote: > >> In D103131#2811969 , >> @kamleshbhalui wrote: >> >>>

[PATCH] D103928: [IR] make -warn-frame-size into a module attr

dblaikie added inline comments. Comment at: llvm/test/Linker/warn-stack-frame.ll:6 + +; CHECK-MISMATCH: error: linking module flags 'warn-stack-size': IDs have conflicting values + If you like, you could make this a function attribute instead, that way it'd be

[PATCH] D104082: [CodeGen] Don't create a fake FunctionDecl when generating block/block_byref copy/dispose helper functions

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D104082#2818281 , @ahatanak wrote: > C++ non-virtual thunks and global initialization functions > (`_GLOBAL__sub_I_*`) have only linkage names.

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added a comment. In D98799#2820198 , @ahatanak wrote: > There are several files other than CGBlocks.cpp in which > `FunctionDecl::Create` is called to create fake FunctionDecls. Do those > places have to be fixed too? This issue in this particu

[PATCH] D104342: [IR] convert warn-stack-size from module attr to fn attr

dblaikie added a comment. what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter? Comment at: llvm/lib/CodeGen/PrologEpilogInse

[PATCH] D104291: [Debug-Info] strict dwarf for DW_LANG_C_plus_plus_14

dblaikie added a comment. In D104291#2823594 , @shchenz wrote: > In D104291#2821581 , @stuart wrote: > >>> There is no `CPlusPlus03` in `LangOptions`, so it is better not to merge >>> `DW_LANG_C_plus_plus_03` sup

[PATCH] D104342: [IR] convert warn-stack-size from module attr to fn attr

dblaikie added a comment. In D104342#2825357 , @nickdesaulniers wrote: > In D104342#2820811 , @dblaikie > wrote: > >> what're the rules about module level attributes like this? For instance: >> Does this affect

[PATCH] D104342: [IR] convert warn-stack-size from module attr to fn attr

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds OK to me. Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have diff

[PATCH] D104342: [IR] convert warn-stack-size from module attr to fn attr

dblaikie added a comment. In D104342#2828193 , @nickdesaulniers wrote: > In D104342#2827847 , @dblaikie > wrote: > >> Another thing you might want to check is linkonce_odr functions - I guess >> you'll get an a

[PATCH] D104342: [IR] convert warn-stack-size from module flag to fn attr

dblaikie added a comment. In D104342#2831733 , @nickdesaulniers wrote: > In D104342#2831717 , @dblaikie > wrote: > >> I don't know that there's a good answer (in more extreme cases - like >> different optimizat

[PATCH] D104601: [Preprocessor] Implement -fnormalize-whitespace.

dblaikie added a comment. This is probably more @aaron.ballman 's wheelhouse, but for my money this seems pretty problematic - will make quoted text in compiler diagnostics weird/difficult to read, etc. Do you have a particular use case that has exceptionally frequent whitespace-only changes?

[PATCH] D104601: [Preprocessor] Implement -fnormalize-whitespace.

dblaikie added a comment. One of the concerns I'd have, for instance (have you done some broad testing of these patches on sizable code bases?) is that it wouldn't surprise me if clang had some scalability bugs/issues with very long source lines - so it might be necessary to introduce some (arb

[PATCH] D104619: [clang] [WIP] Fix for https://bugs.llvm.org/show_bug.cgi?id=50774

dblaikie added a comment. This'll need a test case & does the change pass all existing tests? Also, the patch description could use more detail - it can refer to the bug for more context, but there should be enough detail in the patch title/description to understand the general purpose, etc. (an

[PATCH] D104619: [clang] [WIP] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

dblaikie added a comment. In D104619#2831956 , @nridge wrote: > Thanks for having a look! > > In D104619#2831953 , @dblaikie > wrote: > >> This'll need a test case > > Definitely. Do you have a suggestion for wha

[PATCH] D104342: [IR] convert warn-stack-size from module flag to fn attr

dblaikie added a subscriber: aaron.ballman. dblaikie added a comment. In D104342#2834119 , @nickdesaulniers wrote: > In D104342#2831738 , @dblaikie > wrote: > >>> In D104342#2831717

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. Huh, that surprises me - guess gdb favors checking the symbol first. I guess maybe it is using something that determines that that symbol comes from the file with debug info - because on a similar test case (one file without debug info, defining some global variable `i

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. In D103131#2835004 , @kamleshbhalui wrote: > In D103131#2834744 , @dblaikie > wrote: > >> Huh, that surprises me - guess gdb favors checking the symbol first. I guess >> maybe it is us

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. In D103131#2835702 , @kamleshbhalui wrote: > Here is what we get when we replace int with float. > > $lldb-11 ./a.out > (lldb) target create "./a.out" > Current executable set to > '/folk/kkumar/tcllvm/llvm-build-lldb-re

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

dblaikie added a subscriber: aaron.ballman. dblaikie added a comment. @aaron.ballman Do we have attributes/infrastructure for attributes that have to be the same from their first declaration or at least from their first call? I'm wondering if it might be simpler/better to require nodebug to be p

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. In D103131#2835981 , @kamleshbhalui wrote: > In D103131#2835909 , @dblaikie > wrote: > >> In D103131#2835702 , >> @kamleshbhalui wrote: >> >>>

[PATCH] D104342: [IR] convert warn-stack-size from module flag to fn attr

dblaikie added a comment. In D104342#2836222 , @aaron.ballman wrote: > In D104342#2834240 , @dblaikie > wrote: > >> In D104342#2834119 , >> @nickdesaulniers wrote: >> >

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

dblaikie added a comment. In D104777#2836327 , @aaron.ballman wrote: > In D104777#2835965 , @dblaikie > wrote: > >> @aaron.ballman Do we have attributes/infrastructure for attributes that have >> to be the same

[PATCH] D103131: support debug info for alias variable

dblaikie added a comment. Do you have any interest in seeing if gdb could/would be fixed to handle the imported declaration style? Might be worth knowing if that's feasible, if they have some ideas about if/why that would be a bad idea, etc. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

dblaikie added a comment. In D104777#2837347 , @brunodefraine wrote: > In D104777#2836669 , @dblaikie > wrote: > >> Yeah, all that sounds reasonable to me - @brunodefraine could you look into >> supporting node

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

dblaikie added a comment. In D104777#2838211 , @aaron.ballman wrote: > In D104777#2837794 , @dblaikie > wrote: > >> In D104777#2837347 , >> @brunodefraine wrote: >> >>>

[PATCH] D104883: [CodeGen] Add ParmVarDecls to FunctionDecls that are created to generate ObjC property getter/setter functions

dblaikie added a comment. Ah - this is an alternative to D104082 ? OK. Yeah, looks like it'd address the thing we were discussing in D98799 . But do you have some more details on why D104082 wa

[PATCH] D107155: [clang][deps] Substitute clang-scan-deps executable in lit tests

dblaikie added a comment. Hmm, don't we have some other substitution feature that avoids the need to add the %? (like, I think LLVM's tests don't usually use the % for many tool names - but they still make for good copy/pastable command lines, if I'm not mistaken?) Repository: rG LLVM Githu

[PATCH] D105907: [CallGraphSection] Add call graph section options and documentation

dblaikie added a comment. Looks like the RFC threads didn't have any discussion - I think that'll be necessary before moving forward with/committing any work here, for what it's worth. Perhaps some Apple folks would have some interest since they use ARM and sanitizers a lot & probably care abou

[PATCH] D107155: [clang][deps] Substitute clang-scan-deps executable in lit tests

dblaikie added a comment. In D107155#2917400 , @dexonsmith wrote: > In D107155#2916664 , @dblaikie > wrote: > >> Hmm, don't we have some other substitution feature that avoids the need to >> add the %? (like, I

[PATCH] D107155: [clang][deps] Substitute clang-scan-deps executable in lit tests

dblaikie added a comment. Just because I was curious. Some grep/seding. Nothing definitive: 10188 %clang 1289 %clang_analyze_cc1 32253 %clang_cc1 22 clang-check 503 %clang_cl 59 clang-offload-bundler 2 clang-offload-wrapper 69 clang-rename 2 clang-repl 5

[PATCH] D107155: [clang][deps] Substitute clang-scan-deps executable in lit tests

dblaikie accepted this revision. dblaikie added a comment. Sounds good to me CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107155/new/ https://reviews.llvm.org/D107155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

dblaikie added a comment. Not a huge objection - but minor quandry: What's the motivation for this patch? I don't know of any codebase that encourages/uses the alternative tokens and I wonder if adding more usability to them is a worthwhile investment in clang's codebase complexity, etc. Repo

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

dblaikie added a comment. In D107292#2920746 , @cjdb wrote: > In D107292#2920637 , @dblaikie > wrote: > >> Not a huge objection - but minor quandry: What's the motivation for this >> patch? I don't know of any c

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

dblaikie added a comment. In D106394#2921830 , @kimgr wrote: > Took me a while to get my head around it, but I see now that this is only > supported for system headers. I think that makes sense for the compiler, > otherwise it will be hard to guess whic

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

dblaikie added a comment. In D107292#2921901 , @aaron.ballman wrote: > In D107292#2920774 , @dblaikie > wrote: > >> In D107292#2920746 , @cjdb wrote: >> >>> In D107292#2

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

dblaikie added a comment. In D107292#2922599 , @aaron.ballman wrote: > In D107292#2922575 , @dblaikie > wrote: > >> In D107292#2921901 , >> @aaron.ballman wrote: >> >>>

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

dblaikie added a comment. In D106394#2922972 , @rsmith wrote: > ... As an alternative, have you considered checking whether any of the > headers named in the pragma are in the include stack, and warning if not? > That would seem to make this warning app

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

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

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

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

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

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

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

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

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

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

[PATCH] D104883: [CodeGen] Add ParmVarDecls to FunctionDecls that are created to generate ObjC property getter/setter functions

dblaikie added a comment. In D104883#2841931 , @ahatanak wrote: > I see `assert(DC && "This decl is not contained in a translation unit!");` > fail in `Decl::getTranslationUnitDecl` when `DeclRefExpr` is constructed. > That's because the `ImplicitParamD

[PATCH] D104601: [Preprocessor] Implement -fnormalize-whitespace.

dblaikie added a comment. > Admittedly, I focused on changes (of Clang and Polly) like refactoring, > improving comments, minimize difference to upstream (clang-)formatting etc. > during the testing. Yeah, I was more curious about general purpose/average changes, but anyway. > In D104601#28319

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D104777#2844077 , @brunodefraine wrote: > That I already encounter this in builtin headers is probably an indication > that the new error will

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

dblaikie added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:10466 if (isa(D)) - return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr, D); + if (!isa(D) || + !dyn_cast(D)->getType()->isReferenceType()) cjdb wrote: > db

[PATCH] D102356: [UniqueLinkageName] Use exsiting GlobalDecl object instead of reconstructing one.

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

[PATCH] D103615: [Clang] Add option to handle behaviour of vector bool/vector pixel.

dblaikie added a comment. As mentioned in the reverting commit - these tests fail in non-asserts builds, because they assume named IR instructions (like the named entry BB label), which aren't provided on a non-asserts build (there's a flag to turn them on - but that's probably not the right fi

[PATCH] D104619: [clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

dblaikie added a comment. The test code seems a bit overly general/overengineered for the use case, perhaps? (for instance it has unused parameters like the "Args" parameter - that neither caller passes in a value for, it has accessors (GetPRinted/getNumFoundTypes) when all the code fits on a (

[PATCH] D103615: [Clang] Add option to handle behaviour of vector bool/vector pixel.

dblaikie added a comment. In D103615#2846251 , @dyung wrote: > In D103615#2846245 , @dblaikie > wrote: > >> As mentioned in the reverting commit - these tests fail in non-asserts >> builds, because they assume n

[PATCH] D104619: [clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

dblaikie added a comment. In D104619#2846255 , @nridge wrote: > I based the infrastructure closely on that in `DeclPrinterTest.cpp` and > `StmtPrinterTest.cpp`. I figured the general framework could come in useful > to people adding new type printing te

[PATCH] D104619: [clang] Respect PrintingPolicy::FullyQualifiedName when printing a template-id

dblaikie added a comment. In D104619#2846259 , @nridge wrote: > In D104619#2846258 , @dblaikie > wrote: > >> if that's the case, maybe there's room to refactor the commonality - >> avoiding the near(if it is nea

[PATCH] D103615: [Clang] Add option to handle behaviour of vector bool/vector pixel.

dblaikie added a comment. In D103615#2847118 , @bmahjour wrote: > In D103615#2847047 , @stefanp wrote: > >> I'm sorry I missed the asserts requirement. >> I will recommit this patch after I add `REQUIRES: asserts`

[PATCH] D103615: [Clang] Add option to handle behaviour of vector bool/vector pixel.

dblaikie added a comment. In D103615#2847704 , @bmahjour wrote: >> (generally: disabling the test in non-asserts builds isn't the right path, >> modifying the test so it doesn't depend on asserts IR naming is the right >> path) > > Agreed. > >> Yes, pro

[PATCH] D103615: [Clang] Add option to handle behaviour of vector bool/vector pixel.

dblaikie added a comment. In D103615#2847965 , @stefanp wrote: > In D103615#2847650 , @dblaikie > wrote: > >> In D103615#2847118 , @bmahjour >> wrote: >> >>> In D103615#

[PATCH] D103040: Print default template argument if manually specified in typedef declaration.

dblaikie added inline comments. Comment at: clang/test/SemaTemplate/class-template-id.cpp:12-14 +return ptr2; // expected-error{{cannot initialize return object of type 'A *' (aka 'A *') with an lvalue of type 'const A *'}} else { +return ptr3; // expected-error{{can

[PATCH] D103040: Print default template argument if manually specified in typedef declaration.

dblaikie added inline comments. Comment at: clang/test/SemaTemplate/class-template-id.cpp:12-14 +return ptr2; // expected-error{{cannot initialize return object of type 'A *' (aka 'A *') with an lvalue of type 'const A *'}} else { +return ptr3; // expected-error{{can

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added a comment. In D98799#2850682 , @ahatanak wrote: > I think I've fixed all the places in CodeGen that create fake `FunctionDecl`s > and would cause clang to crash. Thanks, I really appreciate it! I'll have a go at unifying this mangled V un

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

dblaikie added a comment. Looks generally good - few things might be able to be tidied up/simplified, I think? Comment at: clang/unittests/AST/ASTPrint.h:61-62 +private: + // Can be specialized for specific node types. + bool shouldIgnoreNode(const NodeType *N) { return fals

[PATCH] D105395: [IRBuilder] Add type argument to CreateMaskedLoad/Gather

dblaikie added a comment. Might use some testing? (guess there was previously missing testing, or this API change would've broken those tests) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105395/new/ https://reviews.llvm.org/D105395

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

dblaikie added a comment. In D98799#2850700 , @dblaikie wrote: > In D98799#2850682 , @ahatanak wrote: > >> I think I've fixed all the places in CodeGen that create fake >> `FunctionDecl`s and would cause clang to c

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

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

[PATCH] D105835: [Driver] Let -fno-integrated-as -gdwarf-5 use -fdwarf-directory-asm

dblaikie added a comment. Not sure I quite follow - is this an alternative to/replacement for D105662 ? Is the intent to revert D105662 after this (D105835 ) (& some corresponding change to ll

[PATCH] D105835: [Driver] Let -fno-integrated-as -gdwarf-5 use -fdwarf-directory-asm

dblaikie accepted this revision. dblaikie added a comment. Sure, sounds OK. Please make the corresponding change to llc and revert the other patch after this one & the llc one are done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105835/new/ htt

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

dblaikie added subscribers: lhames, dblaikie. dblaikie added a comment. +@lhames for context as the author/owner of `llvm::Error` and associated things. Perhaps it'd be handy to have some descriptions of the problems you encountered, @kuhnel, and how you went about trying to resolve them, to hel

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