[PATCH] D50154: [clangd] capitalize diagnostic messages

dblaikie added subscribers: ilya-biryukov, dblaikie. dblaikie added a comment. What's the motivation for clangd to differ from clang here? (& if the first letter is going to be capitalized, should there be a period at the end? But also the phrasing of most/all diagnostic text isn't in the form of

[PATCH] D50945: [Lex] Make HeaderMaps a unique_ptr vector

dblaikie added a comment. In https://reviews.llvm.org/D50945#1205337, @kristina wrote: > Given the context (class an file name itself) and documentation around the > function, I don't think in this particular case it improves readability or > maintainability, the lifetime of the `HeaderMap` is

[PATCH] D50945: [Lex] Make HeaderMaps a unique_ptr vector

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rC Clang https://reviews.llvm.org/D50945 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D46998: [XRay][clang+compiler-rt] Make XRay depend on a C++ standard lib

dblaikie added a comment. "In the future we can revisit this when we have a better idea as to why not depending on the C++ ABI functionality is a better solution." - this was discussed previously in the thread linked from the bug. A big thing, so far as I understand it, is that Clang doesn't r

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

dblaikie added a comment. > I would prefer to eliminate the `` from the instance name as well, > because our debugger reconstructs a name more to its liking from the > parameter children. However, IIUC the name with params is used for > deduplication in LTO, so that is probably not such a good

[PATCH] D14358: DWARF's forward decl of a template should have template parameters.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks OK to me - couple of minor questions. Comment at: include/clang/Frontend/CodeGenOptions.def:222 ///< of inline stack frames wit

[PATCH] D51177: [DEBUGINFO] Add support for emission of the debug directives only.

dblaikie added inline comments. Comment at: test/Driver/debug-options.c:259 // +// This tests asserts that "-glineinfo-only" "-g0" disables debug info. +// GLIO_NO: "-cc1" 'lineinfo' seems like two words - the inconsistency with 'line-tables' seems like it'd be

[PATCH] D51177: [DEBUGINFO] Add support for emission of the debug directives only.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Not sure that every test for line-tables-only needs to also test line-directives-only, but not a huge deal either way. (only the cases where there's actually a different codepath to test/n

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me & the answers to others various questions seem well addressed. Repository: rC Clang https://reviews.llvm.org/D51576 ___ c

[PATCH] D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support.

dblaikie added a comment. In https://reviews.llvm.org/D51554#1224049, @echristo wrote: > The change in name here from "line tables" to "directives only" feels a bit > confusing. "Limited" seems to be a bit more clear, or even remaining line > tables only. Can you explain where you were going w

[PATCH] D42370: Issue local statics in correct DWARF lexical scope

dblaikie added a comment. Just to clarify - the general philosophy (there are many cases of exceptions due to complications in various areas) is that clang changes should have clang tests (source code to IR), LLVM changes should have LLVM tests (IR to assembly or object code+dwarf dump) & if de

[PATCH] D49916: [CodeGen] Add to emitted DebugLoc information about coverage when it's required

dblaikie added a comment. Test coverage? Repository: rC Clang https://reviews.llvm.org/D49916 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

dblaikie added a comment. Hi - sorry about the stalled opaque pointer types effort. For my money - ideally - if someone comes across a bug caused by incorrect pointee types, ideally that would be fixed by adjusting whatever piece of code was depending on that pointee type being correct to not d

[PATCH] D42813: [Debug] Annotate compiler generated range-for loop variables.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems good, thanks :) Comment at: test/CodeGenCXX/debug-for-range-scope-hints.cpp:1 +// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s + -

[PATCH] D42813: [Debug] Annotate compiler generated range-for loop variables.

dblaikie added a comment. In https://reviews.llvm.org/D42813#1007865, @mattd wrote: > Thanks @dblaikie! I renamed the test, and cleaned up per your suggestion. I > originally regex'd the debug-info lines so that the test would verify that > the names were artificial; however, being that we al

[PATCH] D33437: Emit available_externally vtables opportunistically

dblaikie added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:1383-1385 + if (!OpportunisticVTables.empty()) +assert(shouldOpportunisticallyEmitVTables() && + "Only emit opportunistic vtables with optimizations"); Perhaps this: assert

[PATCH] D33711: [TableGen] Clang changes to support Record::getValueAsString and getValueAsListOfStrings returning StringRef instead of std::string

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Few questions, but address them as you see fit. Comment at: utils/TableGen/ClangDiagnosticsEmitter.cpp:1280-1281 writeHeader((IsRemarkGroup ? "-R" : "-W") + -

[PATCH] D33705: [CGVTables] Finalize SP before attempting to clone it

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. I guess this would need a cross-project test case (ie: it'd have to run LLVM optimizations to fail/pass/demonstrate the fix). I think it'd be OK to add one if there's a neat/clean/obvious

[PATCH] D33102: [clang] Implement -Wcast-qual for C++

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. I still feel like that's more testing than would be ideal (does the context of the cast matter? Wether it's dereferenced, a struct member access, assigned, initialized, etc - it doesn't lo

[PATCH] D33941: [Driver] Add test to cover case when LSan is not supported

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rL LLVM https://reviews.llvm.org/D33941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

dblaikie added a comment. I take it there's already handling for these attributes on non-member functions? There's probably a reason this code can't be wherever that code is & subtract one from the index? (reducing code duplication by having all the error handling, etc, in one place/once) htt

[PATCH] D34052: [XRay][clang] Support capturing the implicit `this` argument to C++ class member functions

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks fine - thanks https://reviews.llvm.org/D34052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D41357: WIP: Fix Diagnostic layering, moving diagnostics out of Basic

dblaikie created this revision. dblaikie added a reviewer: rsmith. Herald added subscribers: cfe-commits, klimek. This goes part-way down the path of moving the actual diagnostics out of Clang's Basic library into the respective libraries that use those diagnostics. The end goal would be that a

[PATCH] D41387: Remove llvm::MemoryBuffer const_casts

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems good Repository: rC Clang https://reviews.llvm.org/D41387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D41357: WIP: Fix Diagnostic layering, moving diagnostics out of Basic

dblaikie added a comment. Updated with a functional implementation - a few points: - Some tests could use only the Common diagnostics (see one of the intermediate changes where I moved those tests over to use all diagnostics) except for the tablegen required for the diagnostic groups makes that

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

dblaikie added a subscriber: Higuoxing. dblaikie added a comment. Probably CC someone from apple here & ask about rdar://8678458 - they can look it up & provide the missing context. https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-co

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

dblaikie added a comment. Not sure I should get too involved in this discussion, knowing so little about Objective C - but if it seems sensible, could you provide some examples (just in comments/description in this code review) of what the DWARF used to look like and what it looks like after th

[PATCH] D47953: [builtin] Add bitfield support for __builtin_dump_struct

dblaikie added a comment. Yeah, I know nothing about this dump feature or what's being fixed here - test cases would be great to help motivate/explain. Repository: rC Clang https://reviews.llvm.org/D47953 ___ cfe-commits mailing list cfe-commits

[PATCH] D48426: [clang-cl] Don't emit dllexport inline functions etc. from pch files (PR37801)

dblaikie added a comment. In https://reviews.llvm.org/D48426#1139823, @rnk wrote: > `LangOpts.ModulesCodegen` is very related in spirit to this, but I think we > need a distinct option because that was designed to handle all inline > functions (too much), not just dllexport inline functions. +

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

dblaikie added a comment. @echristo > As far as the standard text here, IMO it was just there in case people didn't > have an objcopy around or don't want to split it. I'm not sure why we would > want the ability. I think others have mentioned - but with distributed build it might be easier t

[PATCH] D53334: [Frontend] Show diagnostics on prebuilt module configuration mismatch too

dblaikie added a comment. Don't know enough about the code to have an opinion on the fix - but in any case this would need a test case, if possible Repository: rC Clang https://reviews.llvm.org/D53334 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D53329: Generate DIFile with main program if source is not available

dblaikie added a comment. @aprantl - yeah, not sure I have any big feels about this (nor do I fully understand it) @yonghong-song - could you explain this maybe in a bit more detail. What behavior does this fix provide? (compared to behavior of existing working cases that don't hit this bug, f

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

dblaikie added a comment. In https://reviews.llvm.org/D38061#1271021, @twoh wrote: > @aprantl It is a debug info. If you compile > test/CodeGenCXX/debug-info-anonymous.cpp file with `clang -g2 -emit-llvm -S > `, you will find debug metadata something like `distinct !DISubprogram(name: > "foo /

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

dblaikie added a comment. In https://reviews.llvm.org/D52296#1272220, @grimar wrote: > Maybe `-gno-dwo`? So we would write `-genable-split-dwarf -gno-dwo`? I'm not sure how everyone else feels about "-g" flags that modify debug behavior (like "-gsplit-dwarf") versus "-f" flags (eg: "-fdebug-t

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

dblaikie added a comment. In https://reviews.llvm.org/D38061#1275845, @twoh wrote: > @dblaikie I see. The problem we're experiencing is that with Clang's naming > scheme we end up having different function name between the original source > and the preprocessed source (as macro expansion change

[PATCH] D55394: Re-order type param children of ObjC nodes

dblaikie added inline comments. Comment at: test/AST/ast-dump-decl.m:90 // CHECK-NEXT: -ObjCProtocol {{.+}} 'P' +// CHECK-NEXT: -ObjCTypeParamDecl {{.+}} col:33 T 'id':'id' steveire wrote: > aaron.ballman wrote: > > It seems strange to me to print out the

[PATCH] D53329: Generate DIFile with main program if source is not available

dblaikie added a comment. Thanks for filing the bug/reducing a test case - this change should include an automated test case that captures this issue (check for other tests that pass -gembed-source to see how this might be tested, possibly expanding the existing test case case or introducing a

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

dblaikie added inline comments. Comment at: include/clang/AST/Expr.h:5103 +using reference = AssociationTy; +using pointer = AssociationTy; +AssociationIteratorTy() = default; aaron.ballman wrote: > riccibruno wrote: > > aaron.ballman wrote: > > > Car

[PATCH] D53329: Generate DIFile with main program if source is not available

dblaikie added a comment. In D53329#1373799 , @yonghong-song wrote: > @dblaikie As I mentioned in one of my earlier comments, after some analysis, > I think this patch definitely not the right way to fix the problem. We just > cannot default to the main

[PATCH] D53940: [Lex] Make MacroDirective::findDirectiveAtLoc take const SourceManager

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks fine Repository: rC Clang https://reviews.llvm.org/D53940 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

dblaikie added a comment. In https://reviews.llvm.org/D52296#1282369, @grimar wrote: > In https://reviews.llvm.org/D52296#1281642, @echristo wrote: > > > Can you elaborate on your motivations and what you're trying to do? > > > > Thanks! > > > We want to see: > > - No extra files. The compiler pr

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

dblaikie added a comment. In https://reviews.llvm.org/D52296#1282587, @probinson wrote: > In https://reviews.llvm.org/D52296#1282458, @dblaikie wrote: > > > I guess in that case your distributed build system would have a constraint > > that it always ships all the object files back to the primar

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

dblaikie added a comment. In https://reviews.llvm.org/D52296#1282603, @probinson wrote: > In https://reviews.llvm.org/D52296#1282589, @dblaikie wrote: > > > In https://reviews.llvm.org/D52296#1282587, @probinson wrote: > > > > > Any distributed build has to make this work, so the paths in the lin

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

dblaikie added a comment. In https://reviews.llvm.org/D53334#1273877, @whisperity wrote: > @dblaikie I have created a test, but unfortunately `%clang_cpp` in LIT > invokes `clang --driver-mode=cpp` which is not the same as if `clang++` is > called. I'm trying to construct the following command-

[PATCH] D53329: Generate DIFile with main program if source is not available

dblaikie added a comment. In https://reviews.llvm.org/D53329#1270035, @yonghong-song wrote: > Sure. Let me provide a little bit more context and what I want to achieve: > > . I have a tool, called bcc (https://github.com/iovisor/bcc) which uses > clang CompilerInvocation interface and > MC

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

dblaikie added a comment. Thanks! - though on reflection I'm going to invoke @echristo again about the naming. It's unfortunately a bit backwards that the pre-standard flag is -gsplit-dwarf and what we're proposing as the standard flag is -fdwarf-fission, when the DWARF standard doesn't use the

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. While I'm not 100% sure about the actual fix - I'm confident enough that @rsmith can provide any further clarification in post-commit. the test case can probably be simplified before commi

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

dblaikie created this revision. dblaikie added reviewers: JDevlieghere, aprantl, probinson. Herald added a subscriber: cfe-commits. This saves a lot of relocations in optimized object files (at the cost of some cost/increase in linked executable bytes), but gold's 32 bit gdb-index support has a bu

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

This revision was automatically updated to reflect the committed changes. Closed by commit rL346439: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch… (authored by dblaikie, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://re

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

dblaikie added a comment. OK - thanks for that. I'm going to make an executive decision on the naming. Let's go with -gsplit-dwarf[=single] (or explicitly -gsplit-dwarf=split, which is the default value when -gsplit-dwarf is specified). Saves adding a new name/flag, avoids the use of the word

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

dblaikie added inline comments. Comment at: include/clang/AST/Expr.h:1700-1701 bool containsNonAscii() const { -StringRef Str = getString(); -for (unsigned i = 0, e = Str.size(); i != e; ++i) - if (!isASCII(Str[i])) +for (auto c : getString()) + if (!isAS

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

dblaikie added inline comments. Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830 + if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ)) +if (StringRef(A->getValue()) == "single") + return Args.MakeArgString(Output.getFilename()); + Arg *FinalOutput = A

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

dblaikie added a comment. In https://reviews.llvm.org/D54166#1295730, @riccibruno wrote: > @dblaikie Thanks for looking at this patch ! > > I have a set of patches shrinking the other statements/expressions. > Can I add you to review some of these too ? Most of them consists of just > moving >

[PATCH] D54399: Move ExprMutationAnalyzer to Tooling/Analysis (1/3)

dblaikie added a comment. Could you fix the modulemap (might amount to reverting the change Eric made in r342827? or maybe it's more involved than that) & validate that the modules build is working with this change (probably undo Eric's change, validate that you see the breakage that Eric was t

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

dblaikie added a comment. Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my head. I seem to recall some discussion about trivial_abi not implying/being strong enough for all the cases that trivial_relocatable sounds like it covers? Do you happen to remember the distinct

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

dblaikie accepted this revision. dblaikie added inline comments. This revision is now accepted and ready to land. Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830 + if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ)) +if (StringRef(A->getValue()) == "single")

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL346789: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use. (authored by dblaikie, committed b

[PATCH] D54243: DebugInfo: Add a driver flag for DWARF debug_ranges base address specifier use.

dblaikie added a comment. Settled on renaming the flag for consistency with, say, -fdebug-types-section, to -fdebug-ranges-base-address (though 'debug' is sort of ambiguous (Clang can produce non-DWARF debug info) but this driver (as opposed to clang-cl) is intended for DWARF emission, not PDB

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

dblaikie added inline comments. Comment at: docs/LanguageExtensions.rst:1096 + equivalent to copying the underlying bytes and then dropping the source object + on the floor. * ``__is_destructible`` (MSVC 2013) Quuxplusone wrote: > rjmccall wrote: > > Quuxplus

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

dblaikie added inline comments. Comment at: docs/LanguageExtensions.rst:1096 + equivalent to copying the underlying bytes and then dropping the source object + on the floor. * ``__is_destructible`` (MSVC 2013) Quuxplusone wrote: > dblaikie wrote: > > Quuxplus

[PATCH] D54526: [AST] Pack BinaryOperator

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! just a few things that could be cleaned up. Comment at: include/clang/AST/Expr.h:3220 - SourceLocation getExprLoc() const LLVM_READONLY { return O

[PATCH] D54526: [AST] Pack BinaryOperator

dblaikie added inline comments. Comment at: include/clang/AST/Stmt.h:572 CastExprBitfields CastExprBits; +BinaryOperatorBitfields BinaryOperatorBits; InitListExprBitfields InitListExprBits; Oh, just a thought - wonder if we could/should have an asse

[PATCH] D54525: [AST] Pack MemberExpr

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Comment at: include/clang/AST/Expr.h:2799-2802 - - friend TrailingObjects; - friend class ASTReader; - friend class ASTStmtWriter; ---

[PATCH] D54524: [AST] Pack UnaryOperator

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Repository: rC Clang https://reviews.llvm.org/D54524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D53329: Generate DIFile with main program if source is not available

dblaikie added a comment. Thanks for this - though it looks like the test program hits an assertion failure (for me at least - before it gets to the interesting point. clang-test: /usr/local/google/home/blaikie/dev/llvm/src/lib/ExecutionEngine/MCJIT/MCJIT.cpp:204: virtual void llvm::MCJIT::gen

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Agreed - looks like this went in untested in r175813 & has never been used. https://reviews.llvm.org/D54576 ___ cfe-commits mailing list cfe-

[PATCH] D53329: Generate DIFile with main program if source is not available

dblaikie added a subscriber: scott.linder. dblaikie added a comment. Thanks! So I can see where/how this fails now - the LLVM backend seems to require that if any file has embedded source, they all do. Would you be able to/would you mind adding a debug info verifier check for this? That'd help

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

dblaikie added a comment. Cool - thanks! Any chance/way to add a test for this that'd show up sooner than the breakage caused by the previous version? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55006/new/ https://reviews.llvm.org/D55006 ___

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

dblaikie added a comment. In D55006#1311398 , @grimar wrote: > In D55006#1311391 , @dblaikie wrote: > > > Cool - thanks! Any chance/way to add a test for this that'd show up sooner > > than the breakage caused by t

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems OK to me CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55085/new/ https://reviews.llvm.org/D55085 ___ cfe-commits mailing list

[PATCH] D55137: Honor -fdebug-prefix-map when creating function names for the debug info.

dblaikie added a comment. Can't say I know much abouth the path remapping functionality - what it's used for, where it's implemented in general, etc - so figure someone with more of that knowledge might be best off reviewing this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55137/new

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

dblaikie accepted this revision. dblaikie added subscribers: labath, klimek. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me - tagging @labath @klimek here in case they have some further context on whether this is the right place for the test. But I'm f

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

dblaikie added inline comments. Comment at: lib/Sema/SemaOverload.cpp:8979 +// has fewer enable_if attributes than Cand2, and vice versa. +if (std::get<0>(Pair) == None) return Comparison::Worse; I'd probably write this as "if (!std::get<0>(Pair))"

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

dblaikie added inline comments. Comment at: lib/Sema/SemaOverload.cpp:8979 +// has fewer enable_if attributes than Cand2, and vice versa. +if (std::get<0>(Pair) == None) return Comparison::Worse; Meinersbur wrote: > dblaikie wrote: > > I'd probably

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Overall I'm not sure this construct/pattern improves readability, but not too fussed either way. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55468/new/

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

dblaikie added inline comments. Comment at: lib/Serialization/ASTReaderDecl.cpp:2922 +// Return false if the number of enable_if attributes is different. +if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue()) + return false; Meinersbur wr

[PATCH] D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism

dblaikie added inline comments. Comment at: lib/ProfileData/InstrProfWriter.cpp:389-397 + auto nameA = A.first; + auto nameB = B.first; + int comp = nameA.compare(nameB); +

[PATCH] D57986: [ProfileData] Sort FuncData before iteration to remove non-determinism

dblaikie accepted this revision. dblaikie added inline comments. This revision is now accepted and ready to land. Comment at: lib/ProfileData/InstrProfWriter.cpp:431-432 + for (const auto &record : OrderedFuncData) { +const auto &name = record.first; +const auto &Func =

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

dblaikie added a comment. Would it be simpler/better to revert all the FlagTrivial work? & use the FlagNonTrivial+composite type to imply trivial? (since FlagnonTrivial has been in-tree longer) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59347/new/ https://re

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

dblaikie added a comment. Pleasue include mention of the bug (PR40276) in the commit message & clarify that while this is useful for some remote compilation models, it's not strictly necessary/the only way to do it (a remote compilation model that keeps relative paths and uses compilation-dir i

[PATCH] D59008: [AMDGPU] Switch default dwarf version to 5

dblaikie added a comment. In D59008#1441903 , @t-tye wrote: > LGTM > > Do we know the state of split DWARF and DWARF compression for DWARF 5 > (compared to DWARF 2)? State of them in what sense? Compression is pretty orthogonal to any DWARF version - i

[PATCH] D59008: [AMDGPU] Switch default dwarf version to 5

dblaikie added a comment. In D59008#1442256 , @t-tye wrote: > In D59008#1442014 , @dblaikie wrote: > > > In D59008#1441903 , @t-tye wrote: > > > > > LGTM > > > > > > Do we kn

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

dblaikie added a comment. @asmith: Where's the LLVM-side change/review that goes with this, btw? In D59347#1442970 , @probinson wrote: > As a rule I would prefer flags with positive names, as it's slightly easier > to read `!isTrivial` than `!isNonTrivia

[PATCH] D59008: [AMDGPU] Switch default dwarf version to 5

dblaikie added a comment. In D59008#1442962 , @probinson wrote: > In D59008#1442515 , @dblaikie wrote: > > > In D59008#1442256 , @t-tye wrote: > > > > > In D59008#1442014

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

dblaikie added a comment. In D59347#1443051 , @dblaikie wrote: > @asmith: Where's the LLVM-side change/review that goes with this, btw? > > In D59347#1442970 , @probinson wrote: > > > As a rule I would prefer flags

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

dblaikie requested changes to this revision. dblaikie added a comment. This revision now requires changes to proceed. What's the general motivation for this work/changes? > -gmlt -gsplit-dwarf -fno-split-dwarf-inlining => special: 1 (before) 2 (after) This ^ is the only semantic change due to th

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

dblaikie added a comment. In D59923#1447198 , @MaskRay wrote: > In D59923#1446508 , @dblaikie wrote: > > > What's the general motivation for this work/changes? > > > The current -gsplit-dwarf handling is a bit compl

[PATCH] D58497: Clear the KnownModules cache if the preprocessor is going away

dblaikie added a comment. In D58497#1449243 , @nemanjai wrote: > Ping. Unfortunately Richard Smith is out for a few weeks at the moment, so might take a little bit before he can get to this. It's odd to me that this lacks a test case - but you mention

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

dblaikie added a comment. In D59673#143 , @aaronpuchert wrote: > In D59673#1438716 , @dblaikie wrote: > > > Use llvm-dwarfdump rather than llvm-objdump to dump the contents of the > > debug_info section and te

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

dblaikie added a comment. I'm still not entirely clear on how this is all changing, sorry - in the patch description summary, the first block of examples doesn't mention which of those disables split DWARF (the second block of examples all say "+ split" - is that to imply that the first block a

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

dblaikie added a comment. In D59923#1460696 , @MaskRay wrote: > > is that to imply that the first block all do not use split DWARF? > > The first block do not use split DWARF. That doesn't sound like what I'd expect (& would represent a change in behavio

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

dblaikie added a comment. In D59673#1460605 , @aaronpuchert wrote: > Ok, here is an idea. We introduce `-split-dwarf-output` in Clang instead of > `-fsplit-dwarf-dwo-name-attr`. If given, it overrides the output file name > for the Split DWARF file, whi

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Approved pending the LLVM side changes/discussion. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59347/new/ https://reviews.llvm.org/D59347

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

dblaikie added a comment. In D59673#1465975 , @aaronpuchert wrote: > In D59673#1461983 , @dblaikie wrote: > > > Sure, I think the naming's a bit weird (but hard to come up with good names > > for any of this) > >

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

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

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

dblaikie added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:8623 + } else if (A.hasScope()) { +#define ATTR(A) +#define ATTR_NAMESPACE(A) .Case(#A, false) dblaikie wrote: > Not sure how it's done elsewhere - but I'd sink these #defines down to > immed

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems pretty good to me - if you'd like to wait for more/other feedback from @rsmith or the like, that's OK too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60872/new/ https://

[PATCH] D60892: Modules: Search for a visible definition of the decl context when computing visibility of a default template parameter

dblaikie created this revision. dblaikie added a reviewer: rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. The code is/was already correct for the case where a parameter is a parameter of its enclosing lexical DeclContext (functions and classes). But for other templa

[PATCH] D60892: Modules: Search for a visible definition of the decl context when computing visibility of a default template parameter

This revision was automatically updated to reflect the committed changes. Closed by commit rL358795: Modules: Search for a visible definition of the decl context when computing… (authored by dblaikie, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed p

[PATCH] D61079: Skip type units/type uniquing when we know we're only emitting the type once (vtable-based emission when triggered by a strong vtable, with -fno-standalone-debug)

dblaikie created this revision. dblaikie added a reviewer: aprantl. Herald added a project: clang. Herald added a subscriber: cfe-commits. (this would regress size without a corresponding LLVM change that avoids putting other user defined types inside type units when they aren't in their own type

  1   2   3   4   5   6   7   8   9   10   >