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

dexonsmith added a reviewer: bruno. dexonsmith added a comment. +bruno Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58497/new/ https://reviews.llvm.org/D58497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM too. https://reviews.llvm.org/D30882 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

dexonsmith added a comment. I like this idea, and I don’t think the textual IR central is too important. A few things: - Changes to the IR should always be discussed on llvm-dev. Did this already happen? - Please update LangRef. - Did you write a script for updating the test cases? If so, yo

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

dexonsmith added a comment. In D58548#1407164 , @dexonsmith wrote: > I like this idea, and I don’t think the textual IR central is too important. Textual IR *change*; typing on a phone while walking :/. By which I mean that IMO it’s fine to break/chang

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

dexonsmith added a comment. In D58548#1407331 , @greened wrote: > +1. Is there any reason not to use "%4" in the definition? > > define i32 @f(i32, i32) { > %3 = add i32 %0, %1 > br label %4 > > %4: > ret i32 %3 > } > > > Maybe it crea

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

dexonsmith added a comment. In D58548#1407355 , @dexonsmith wrote: > In D58548#1407331 , @greened wrote: > > > +1. Is there any reason not to use "%4" in the definition? > > > > define i32 @f(i32, i32) { > >

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

dexonsmith added a comment. I have a few nitpicks, but otherwise this seems right. I'll wait for the llvm-dev discussion before LGTM'ing though. Comment at: llvm/lib/AsmParser/LLParser.cpp:2930-2931 +if (NameID != -1 && unsigned(NameID) != NumberedVals.size()) { + P.

[PATCH] D58559: emit '(assertions enabled)' in the version string for a build of clang with assertions enabled

dexonsmith added a comment. I like this. Can you start a discussion on cfe-dev (if you haven't already)? This is user-visible so I want to be sure other vendors are happy with this; if not, we can hide it behind a CMake flag. Comment at: lib/Basic/Version.cpp:117 +#ifndef N

[PATCH] D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher

dexonsmith added inline comments. Comment at: clang/lib/DirectoryWatcher/DirectoryWatcher-linux.inc.h:154 +errorMsg += llvm::sys::StrError(); +return true; + }; jkorous wrote: > mgorny wrote: > > The return values seem to be confusing. Any reason not to

[PATCH] D64600: [ObjC] Add an attribute that "clamps" signed char BOOLs to {0,1}

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/Attr.td:3279 +def ObjCClampingBool : TypeAttr { + let Spellings = [Clang<"objc_clamping_bool">]; + let Subjects = SubjectList<[TypedefName]>; erik.pilkington wrote: > aaron.ballman wrote: >

[PATCH] D64945: [clang-scan-deps] Dependency directives source minimizer: handle #pragma once

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM after you add the testcase I mention. Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:623 +// #pragma once +skipLine(First, End); +

[PATCH] D65906: [clang-scan-deps] Fix edge cases in the minimizer

dexonsmith added a comment. In D65906#1621542 , @aganea wrote: > In D65906#1620034 , @arphaman wrote: > > > Regarding the invisible characters before `#ifdef`, are you sure that's the > > right behavior? > > > Shou

[PATCH] D63048: Update __VERSION__ to remove the hardcoded 4.2.1 version

dexonsmith added a comment. In D63048#1558806 , @sylvestre.ledru wrote: > @dexonsmith ping? I think I'd be a little more comfortable dropping `__VERSION__` entirely. There's already a way to access the clang version, and anyone with code that relies

[PATCH] D64149: [clang-scan-deps] use `-Wno-error` when scanning for dependencies

dexonsmith added a comment. Is warning output suppressed? If so, should we just/also disable all warnings? (IIRC, the flag is `-w`.) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64149/new/ https://reviews.llvm.org/D64149

[PATCH] D64525: [clang-scan-deps] Dependency directives source minimizer: single quotes are not digit separators after a valid character literal prefix

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, but I have a suggestion inline for another approach. Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:265-272 + // Make sure that the L, u, U,

[PATCH] D62458: Frontend: Add CompilerInvocation and DiagnosticsEngine to the builder

dexonsmith created this revision. dexonsmith added a reviewer: rsmith. Herald added subscribers: kadircet, arphaman, jkorous. dexonsmith added a parent revision: D62457: Frontend: Create basic CompilerInstanceBuilder. Add CompilerInvocation and DiagnosticsEngine to the new builder, replacing a nu

[PATCH] D62457: Frontend: Create basic CompilerInstanceBuilder

dexonsmith created this revision. dexonsmith added a reviewer: rsmith. dexonsmith added a child revision: D62458: Frontend: Add CompilerInvocation and DiagnosticsEngine to the builder. This sketches out the beginning of a `CompilerInstanceBuilder`. For now it just handles PCHContainerOps and InM

[PATCH] D62493: [Driver] Always use Unix-style paths in the Darwin driver

dexonsmith added a comment. This doesn’t look okay to me, because this would prevent building for Darwin when running on Windows. I added a couple of reviewers that have Windows experience and might have ideas for how to fix the tests without breaking the portability of the driver. Repositor

[PATCH] D55455: Remove stat cache chaining as it's no longer needed after PTH support has been removed

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. It has been about a week since your cfe-dev post with no concerns. I think you can commit. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55455/new/ h

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

dexonsmith added inline comments. Comment at: clang/test/Sema/pragma-attribute-label.c:7 + +#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}} +#pragma clang attribute pop NOT_MY_LABEL // expected-error{

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

dexonsmith added inline comments. Comment at: clang/test/Sema/pragma-attribute-label.c:7 + +#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop' with no matching '#pragma clang attribute push'}} +#pragma clang attribute pop NOT_MY_LABEL // expected-error{

[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

dexonsmith added inline comments. Comment at: clang/test/Sema/pragma-attribute-label.c:15 +// Out of order! +#pragma clang attribute pop MY_LABEL + aaron.ballman wrote: > dexonsmith wrote: > > erik.pilkington wrote: > > > aaron.ballman wrote: > > > > dexonsmith w

[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

dexonsmith added a comment. In D55869#1337692 , @rjmccall wrote: > Okay. That's also presumably true for open-source runtimes that support ARC; > tagging David Chisnall and Jonathan Schleifer to get their input. `shouldUseARCFunctionsForRetainRelease()

[PATCH] D55869: Convert some ObjC retain/release msgSends to runtime calls

dexonsmith added a comment. In D55869#1337706 , @js wrote: > The ObjFW runtime itself does not contain anything about release, retain or > autorelease - these are all part of ObjFW (as in the framework). As ObjFW > also supports the Apple runtime, as wel

[PATCH] D56226: [clang-format] square parens that are followed by '=' are not Objective-C message sends

dexonsmith added a comment. Note that a message send needs to have two expressions without an operator in between. Can we also rule out all square brackets that just have a single identifier or number token? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56226/n

[PATCH] D56523: Improve a -Wunguarded-availability note

dexonsmith added inline comments. Comment at: clang/test/Sema/availability-guard-format.mm:6 @interface foo -- (void) method_bar __attribute__((availability(macosx, introduced = 10_12))); // expected-note {{'method_bar' has been explicitly marked partial here}} +- (void) method

[PATCH] D68052: [clang-scan-deps] Allow continuation line backslashes followed by whitespace in the dependency source minimizer

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. The code looks correct. I have some nitpicks about how the functions are named, but you don't need to go with my suggestions specifically. Comment at: clang/lib/Lex

[PATCH] D68052: [clang-scan-deps] Allow continuation line backslashes followed by whitespace in the dependency source minimizer

dexonsmith added inline comments. Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:247-248 -static const char *reverseOverSpaces(const char *First, const char *Last) { +static const char *reverseOverSpacesUntilFirstSpace(const char *First, +

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

dexonsmith requested changes to this revision. dexonsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:21-22 // Load the file and its content from the file system. ll

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

dexonsmith added a comment. Sorry for bouncing you around, but I just had a look at the other user of `createFileEntry` and I think the right thing to do is to somehow share the code between `DependencyScanningWorkerFilesystem::status` and this. I suggest splitting a function out of `status` (

[PATCH] D68436: [clang-scan-deps] Improve string/character literal skipping

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with a few nitpicks about unnecessary nesting. Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:205 + return; +if (*First == '\\') { +

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Hal requested splitting the patch in two, since the two features are separable, but they both still seem to be here. Perhaps start with the prefix patch?

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Herald added a subscriber: kbarton. I don't think this is quite right. I know at least `make`-based incremental builds wouldn't deal well with this. Given t.cpp: #if __ha

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

dexonsmith added a comment. In https://reviews.llvm.org/D30882#1075407, @ddunbar wrote: > In https://reviews.llvm.org/D30882#1074822, @dexonsmith wrote: > > > I don't think this is quite right. I know at least `make`-based > > incremental builds wouldn't deal well with this. > > > This is actua

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

dexonsmith added a comment. In https://reviews.llvm.org/D30882#1075576, @pete wrote: > Oh, that actually wasn't my intention when I wrote it. > > Honestly I didn't expect it to log anything for missing paths at all, as we > don't currently log all the missing (but attempted) paths for regular >

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

dexonsmith added a comment. In https://reviews.llvm.org/D30882#1075589, @dexonsmith wrote: > In https://reviews.llvm.org/D30882#1075576, @pete wrote: > > > Would it be ok to turn this on by default, without a flag, only in the case > > of the path actually existing, and only the found path being

[PATCH] D47906: [ThinLTO] Add testing of summary index parsing to a couple CFI tests

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

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

dexonsmith added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3355 + QualType QTy(ID->getTypeForDecl(), 0); + auto it = TypeCache.find(QTy.getAsOpaquePtr()); + if (it != TypeCache.end()) { LLVM style rules suggest UpperCamelCase or INIT

[PATCH] D48675: [libc++abi] Limit libc++ header search to specified paths

dexonsmith added subscribers: beanz, EricWF. dexonsmith added a comment. Please close this and open a new one. Adding cfe-commits after the fact will fail to send the patch/description to cfe-commits, and not everyone is on llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D48675

[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

dexonsmith added a reviewer: ldionne. dexonsmith added a comment. What's the effective change on the command-line? Does this map to `-nostdinc`? To `-nostdinc++`? Repository: rCXXA libc++abi https://reviews.llvm.org/D48694 ___ cfe-commits mail

[PATCH] D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY

dexonsmith added a comment. In https://reviews.llvm.org/D48892#1153473, @davide wrote: > The lldb bot started failing very recently and the blamelist hints at this > change. > > http://green.lab.llvm.org/green/job/lldb-cmake// > > Can you please take a look? > > For your convenience, this is

[PATCH] D49051: [libclang] check that the cursor is declaration before trying to evaluate the cursor like a declaration

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Comment at: tools/libclang/CIndex.cpp:3892-3922 CXEvalResult clang_Cursor_Evaluate(CXCursor C) { - const Decl *D = getCursorDecl(C); - if (D) { -const Ex

[PATCH] D68193: In openFileForRead don't cache erroneous entries if the error relates to them being directories. Add tests.

dexonsmith accepted this revision. dexonsmith added a comment. LGTM too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68193/new/ https://reviews.llvm.org/D68193 ___ cfe-commits mailing list cfe-commit

[PATCH] D68835: [clang-scan-deps] Add basic support for Clang modules.

dexonsmith added inline comments. Comment at: lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:135 + return llvm::StringSwitch(Ext) +.CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", true) +.CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", true)

[PATCH] D68914: [clang-format] Remove duplciate code from Invalid BOM detection

dexonsmith added a comment. In D68914#1710407 , @owenpan wrote: > I did a quick search of the LLVM code base and found dozens of `const > StringRef &`, including the `Twine` API >

[PATCH] D56924: Special case ObjCPropertyDecl for printing

dexonsmith added subscribers: jkorous, benlangmuir. dexonsmith added inline comments. Comment at: unittests/AST/NamedDeclPrinterTest.cpp:220 +"property", +"Obj::property")); +} gribozavr wrote: > I don't think that `Obj::property` is the preferred syntax.

[PATCH] D69090: [Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

dexonsmith added a comment. In D69090#1713983 , @kousikk wrote: > Thanks for the comment @jkorous. > > > I think you could've just used CHECK-DAG to fix the tests. It *might* be a > > bit more robust. Although just reordering checks seems perfectly fine t

[PATCH] D80383: Add AST_SIGNATURE record to unhashed control block of PCM files

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Thanks for working on this! I have a number of suggestions inline. Comment at: clang/include/clang/Serialization/ASTBitCodes.h:44 /// AST files at this

[PATCH] D77586: Allow parameter names to be elided in a function definition in C

dexonsmith added a comment. In D77586#1967052 , @erik.pilkington wrote: > > This also adds the same feature to ObjC blocks under the assumption that > > ObjC wishes to follow the C standard in this regard. > > SGTM - thats generally been the idea for blo

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dexonsmith added a comment. This is thanks to a commit of mine that shaved a word off of `SmallVector`. Some options to consider: 1. Revert to word-size integers (`size_t`? `uintptr_t`?) for Size and Capacity for small-enough types. Could be just if `sizeof(T)==1`. Or maybe just for `char`

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Requesting changes just to be sure we consider the other options. I don't think it's good that `SmallVector` is no longer useful for large byte streams; I would prefer to fi

[PATCH] D77697: libc++: adjust modulemap for non-modular C

dexonsmith added a comment. This SGTM in principle, but the patch makes me wonder: which other headers from the non-modularized C runtime change ownership? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77697/new/ https://reviews.llvm.org/D77697

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dexonsmith added a comment. In D77621#1968647 , @browneee wrote: > Do we want to increase the complexity of SmallVector somewhat? or do we want > to keep the limit and affirm SmallVector is for small things? I don't think we should limit `SmallVector` t

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

dexonsmith added a comment. In D70366#1970375 , @jdoerfert wrote: > TBH, I would issue a warning if we see `flatten` in O0 that says this will > not work and be done with it. I would argue against diagnostics that depend on optimization level, since tha

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

dexonsmith added a comment. In D70366#1970299 , @LevitatingLion wrote: > Maybe we can add an additional string attribute when adding the noinline > attribute to functions which are not marked noinline in the source code, > something like "noinline-added

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

dexonsmith added a comment. In D70366#1970526 , @dexonsmith wrote: > In D70366#1970299 , @LevitatingLion > wrote: > > > Maybe we can add an additional string attribute when adding the noinline > > attribute to fun

[PATCH] D77697: libc++: adjust modulemap for non-modular C

dexonsmith added a subscriber: Bigcheese. dexonsmith added a comment. In D77697#1969998 , @compnerd wrote: > @dexonsmith - yeah, sadly I dont think that there is a good way to audit that > - any change to the public headers can cause issues. Furthermore,

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

dexonsmith added a comment. In D70366#1970758 , @jdoerfert wrote: > In D70366#1970526 , @dexonsmith > wrote: > > > In D70366#1970299 , > > @LevitatingLion wrote: > > > > >

[PATCH] D77772: [Clang] Expose RequiresNullTerminator in FileManager.

dexonsmith added a comment. Code change LGTM. Is there a way to add a `MemoryBuffer` unit test for the change to `shouldUseMmap`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D2/new/ https://reviews.llvm.org/D2

[PATCH] D77772: [Clang] Expose RequiresNullTerminator in FileManager.

dexonsmith added a comment. In D2#1973250 , @Bigcheese wrote: > Not really. It's a static function in MemoryBuffer.cpp, and the > `MemoryBuffer` class doesn't have a `Kind` member so we can't check for > `MemoryBufferMMapFile`. Ah, but there's a vt

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Thanks for your patience, I missed the updates on Friday. I have a couple of optional comments inline that I don't feel strongly about. LGTM either way. In D77621#1972764

[PATCH] D70284: Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

dexonsmith added a comment. In D70284#1752893 , @bendjones wrote: > In D70284#1752811 , @dexonsmith > wrote: > > > For some reason this revision is locked down and I'm not allowed to "edit" > > it, which includes

[PATCH] D77772: [Clang] Expose RequiresNullTerminator in FileManager.

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Comment at: llvm/unittests/Support/MemoryBufferTest.cpp:396 + + auto MBOrError = MemoryBuffer::getOpenFile(FD, TestPath, -1, false, true); + ASSERT_NO_ERROR(M

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dexonsmith added a comment. In D77621#1979183 , @browneee wrote: > @dexonsmith I am not a committer, if the last changes looks good please > submit for me. Thanks! You've had a few patches in the past, I suggest you get yourself access. https://llvm.org

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. In D74813#1982089 , @alexbdv wrote: > @erik.pilkington / @vsk / @dexonsmith - how block name = > hash(parent_function_name + block_type) ? > So any blocks that are inside the same parent function + have the same type > will

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. Yes, I like the look of that. Can you also update the demangler to reverse this nicely? @erik.pilkington can help there. There are two copies. Usually you modify the copy in `libcxxabi/` and then run a script to copy the changes into `llvm/`. Repository: rG L

[PATCH] D80383: Add AST_SIGNATURE record to unhashed control block of PCM files

dexonsmith added a comment. In D80383#2068596 , @dang wrote: > I made the decision to make all relative offsets relative to the start of the > AST block rather than their own sub-block or a neighboring block, in order to > prevent adding complexity in `A

[PATCH] D80849: [apple clang] disable in-process CC1 to preserve crashlog compatibility

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, but next we should get the crash reproductions working with in-process `-cc1`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80849

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

dexonsmith added a comment. I have a couple of drive-by suggestions, up to @dang and @Bigcheese whether to incorporate them. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:137-142 +.Case("static", llvm::Reloc::Static) +.Case("pic", llvm::

[PATCH] D81347: Make ASTFileSignature an array of 20 uint8_t instead of 5 uint32_t

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM once @aprantl is happy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81347/new/ https://reviews.llvm.org/D81347 ___

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

dexonsmith added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3647 + DiagnosticsEngine &Diags) { +#define OPTION_WITH_MARSHALLING +#define OPTION_WITH_MARSHALLING_FLAG(PREFIX_TYPE, NAME, ID, KIND, GROUP, \ -

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

dexonsmith added inline comments. Comment at: clang/include/clang/Driver/CC1Options.td:216-221 def mrelocation_model : Separate<["-"], "mrelocation-model">, - HelpText<"The relocation model to use">, Values<"static,pic,ropi,rwpi,ropi-rwpi,dynamic-no-pic">; + HelpText<"The rel

[PATCH] D80383: Add AST_SIGNATURE record to unhashed control block of PCM files

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with one more comment inline. Comment at: clang/lib/Serialization/ASTWriter.cpp:2345 FirstMacroID - NUM_PREDEF_MACRO_IDS

[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

dexonsmith added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3845 + IS_POSITIVE != DEFAULT_VALUE && this->KEYPATH != DEFAULT_VALUE) \ +Args.push_back(StringAllocator(Twine(PREFIX_TYPE[0]) + NAME)); +#include "clang/Driver/Options.inc"

[PATCH] D83071: Add support for options with two flags for controlling the same field.

dexonsmith requested changes to this revision. dexonsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:150 -static llvm::Optional normalizeSimpleEnum(OptSpecifier Opt, -

[PATCH] D82661: [clang-tidy][NFC} Remove unnecessary includes throughout clang-tidy header files

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. > Plus replacing a few std::map with llvm::StringMap That doesn't sound NFC since it changes ordering semantics. It seems to me like it should be done separately from the res

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

dexonsmith closed this revision. dexonsmith added a comment. In D77621#1979769 , @browneee wrote: > GIT_COMMITTER_NAME=Andrew Browne > GIT_COMMITTER_EMAIL=brown...@google.com > > This would be my second commit. I will request access next time - thanks >

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dexonsmith added a comment. In D77621#1995673 , @browneee wrote: > Thanks for the revert explanation and notes, nikic. > > @dexonsmith what is your current thinking on going back to the original > std::vector approach? SmallVector has only been limited

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dexonsmith added a comment. In D77621#2000237 , @nikic wrote: > Okay, I'm basically fine with that, if it is our stance that SmallVector > should always be preferred over std::vector. Not really related to this > revision, but it would probably help to d

[PATCH] D77621: Change BitcodeWriter buffer to std::vector instead of SmallVector.

dexonsmith added a comment. @nikic, great news! Thanks for doing the detailed analysis. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77621/new/ https://reviews.llvm.org/D77621 ___ cfe-commits mailing

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. In D74813#2010859 , @erik.pilkington wrote: > In D74813#2010767 , @alexbdv wrote: > > > @erik.pilkington would the hash-based numbering be OK for now ? > > > Feel free to drop the demang

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. In D74813#2011083 , @alexbdv wrote: > @dexonsmith what regression are you referring to ? > What this change is effectively doing now is changing the numbering of the > blocks from incremental to hash-based. > So the demangle

[PATCH] D77621: ADT: SmallVector size/capacity use word-size integers when elements are small

dexonsmith added a comment. In D77621#2002430 , @dblaikie wrote: > (seems a bit questionable to rely on uintptr_t being necessarily the same > type as either uint32_t or uint64_t - but maybe that's guaranteed/written > down somewhere)? I think in pract

[PATCH] D79343: [libc++][test] Adjust move_iterator tests to allow C++20

dexonsmith added a comment. In D79343#2023658 , @CaseyCarter wrote: > I'm not going to reformat *only* my additions per the clang-format > instructions - that would be silly - and I suspect that folks would > clang-format all of the tests if they actuall

[PATCH] D79343: [libc++][test] Adjust move_iterator tests to allow C++20

dexonsmith added a comment. In D79343#2025211 , @ldionne wrote: > Regarding the formatting changes, I personally think we should clang-format > all of libc++, libc++abi and libunwind in one go. Then we'd be done with > these small issues that add up. And

[PATCH] D79796: [DO NOT REVIEW] Sketch support for generating CC1 command line from CompilerInvocation

dexonsmith added inline comments. Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:162 // The option identifier name. -OS << ", "<< getOptionName(R); +OS << ", " << getOptionName(R); This whitespace fixup LGTM, but you should commit it separat

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

dexonsmith added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:397 +return false; + return FE->getUID() == SourceFile.getUID(); +} arphaman wrote: > jkorous wrote: > > I don't really understand all the details here but shouldn't we use this

[PATCH] D74813: Function block naming - add hash of parameter type to end of block name

dexonsmith added a comment. In D74813#2036966 , @alexbdv wrote: > @dexonsmith - Are you OK with that ? SGTM. I suggest @erik.pilkington lands the demangler patch before landing this so there isn't a window where we mangle something that can't be demang

[PATCH] D79916: Map -O to -O1 instead of -O2

dexonsmith added reviewers: arphaman, Gerolf. dexonsmith accepted this revision. dexonsmith added a comment. IOW, this LGTM if Alex and Gerolf are happy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79916/new/ https://reviews.llvm.org/D79916 __

[PATCH] D79916: Map -O to -O1 instead of -O2

dexonsmith added subscribers: arphaman, Gerolf. dexonsmith added a comment. In D79916#2039789 , @echristo wrote: > I'm totally down, but you knew that already :) > > Duncan: Do you have any concerns? I doubt it, but just checking. Xcode doesn't use `-O

[PATCH] D80055: Diagnose union tail padding

dexonsmith added inline comments. Comment at: clang/test/CodeGen/union-tail-padding.c:28-36 +union Front { + int i; + long long ll; +}; + +union Front front1; +union Front front2 = {};// expected-warning {{Initializing union 'Front' field 'i' only initializes the first

[PATCH] D80055: Diagnose union tail padding

dexonsmith added inline comments. Comment at: clang/test/CodeGen/union-tail-padding.c:28-36 +union Front { + int i; + long long ll; +}; + +union Front front1; +union Front front2 = {};// expected-warning {{Initializing union 'Front' field 'i' only initializes the first

[PATCH] D80055: Diagnose union tail padding

dexonsmith added inline comments. Comment at: clang/test/CodeGen/union-tail-padding.c:28-36 +union Front { + int i; + long long ll; +}; + +union Front front1; +union Front front2 = {};// expected-warning {{Initializing union 'Front' field 'i' only initializes the first

[PATCH] D79916: Map -O to -O1 instead of -O2

dexonsmith added a comment. In D79916#2039842 , @dexonsmith wrote: > IOW, this LGTM if Alex and Gerolf are happy. Gerolf told me he has no concerns. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79916/new/

[PATCH] D40998: [driver][darwin] Take the OS version specified in "-target" as the target OS instead of inferring it from SDK / environment

dexonsmith requested changes to this revision. dexonsmith added inline comments. This revision now requires changes to proceed. Comment at: lib/Driver/ToolChains/Darwin.cpp:1234-1276 +// The -target option specifies the deployment target when +// -m-version-min is not giv

[PATCH] D41035: [driver][darwin] Refactor the target selection code, NFC

dexonsmith added a comment. This seems correct to me. I wouldn't mind having someone else back me up though. Also, I have a suggestion for a `static_assert`. Comment at: lib/Driver/ToolChains/Darwin.cpp:1334-1339 + const char *EnvVars[Darwin::LastDarwinPlatform + 1] = { +

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block

dexonsmith resigned from this revision. dexonsmith added a comment. Akira and/or John should take a look instead of me. Repository: rC Clang https://reviews.llvm.org/D41050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added a comment. In D121733#3403995 , @rnk wrote: > Ignoring the ".." path components for the moment, is this patch good to go as > is? It doesn't affect that behavior. Besides the test failure, the other thing is considering what to do with

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

dexonsmith added a comment. This looks nice! Comment at: clang/lib/Basic/FileManager.cpp:287-289 // name-as-accessed on the \a FileEntryRef. Maybe the returned \a // FileEntryRef::getName() could return the accessed name unmodified, but // make the external name

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

dexonsmith added a comment. In D122549#3412021 , @bnbarham wrote: > `clang-apply-replacements/relative-paths.cpp` is failing, I haven't looked > into it but my guess would be that it's from the `Status.getName() == > Filename` -> `!Status.IsVFSMapped` c

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