[PATCH] D137304: [clang] Store filename per include instead of mutating filename

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:135-139 /// References the file which the contents were actually loaded from. /// /// Can be different from 'Entry' if we overridden the contents of one file /// with the contents

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:652-662 + llvm::DenseMap + NamedFileInfos; + /// Memoized information about all of the files tracked by this /// SourceManager. /// It feels expensive to have b

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:261-263 + /// FIXME: Make non-optional using a virtual file as needed, remove \c + /// Filename and use \c OrigEntry.getNameAsRequested() instead. + OptionalFileEntryRefDegradesToFileEntryP

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:135-139 /// References the file which the contents were actually loaded from. /// /// Can be different from 'Entry' if we overridden the contents of one file /// with the contents

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

dexonsmith added a comment. In D137473#3915497 , @bnbarham wrote: > This seems reasonable to me in general. @dexonsmith in case you have any > thoughts. SGTM! (I haven't reviewed in detail but I figure @bnbarham is on it...) Repository: rG LLVM Gith

[PATCH] D127408: [clang][driver] Introduce new -fdriver-only flag

dexonsmith accepted this revision. dexonsmith added a comment. LGTM with a minor comment on the test. Comment at: clang/test/Driver/driver-only.c:13 + +// Check that -fdriver-only respects -v. +// Should there be a `-check-epmty` or similar when `-v` is not pas

[PATCH] D128207: [clang-doc][NFC] Fix reference invalidation assertion failure.

dexonsmith added a comment. Is there a test that can be added so this doesn’t regress? Also, I wonder if `assign` could/should add logic to be resilient to this. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128207/new/ https://reviews.llvm.org/D12

[PATCH] D125814: Improve the strict prototype diagnostic behavior

dexonsmith resigned from this revision. dexonsmith added a comment. (I’ll be happy with whatever you two sort out here!) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125814/new/ https://reviews.llvm.org/D125814 ___ cfe-commits mailing list c

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

dexonsmith added inline comments. Comment at: clang/test/Modules/Inputs/System/usr/include/stdint.h:2 typedef int my_awesome_nonstandard_integer_type; + +/* C99 7.18.1.1 Exact-width integer types. Bigcheese wrote: > iana wrote: > > dexonsmith wrote: > > > benlan

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

dexonsmith added a comment. Nice! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74094/new/ https://reviews.llvm.org/D74094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D158137: Change -ffp-model= related warn_drv_overriding_flag_option to warn_drv_overriding_option

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Perhaps as a follow-up, rename warn_drv_overriding_flag_option to have “t” in it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

dexonsmith added a comment. This seems to drop `-Woverriding-t-option` entirely. Could that break builds if someone has (e.g.) `-Werror -Wno-overriding-t-option` in their build settings? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158137/new/ ht

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

dexonsmith added a comment. Can you explain the downside of leaving behind an alias? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158137/new/ https://reviews.llvm.org/D158137 ___ cfe-commits mailing lis

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

dexonsmith added a comment. In D158137#4597009 , @MaskRay wrote: > In D158137#4596948 , @dexonsmith > wrote: > >> Can you explain the downside of leaving behind an alias? > > Two minor ones. (a) Existing `-Wno-ov

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

dexonsmith added a comment. In D158137#4597565 , @MaskRay wrote: > In D158137#4597491 , @dexonsmith > wrote: > >> In D158137#4597009 , @MaskRay >> wrote: >> >>> In D1581

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

dexonsmith added inline comments. Comment at: clang/test/Driver/darwin-version.c:217 // RUN: FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s -// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' +// CHECK-VERSION-

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

dexonsmith added inline comments. Comment at: clang/test/Driver/darwin-version.c:217 // RUN: FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s -// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' +// CHECK-VERSION-

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

dexonsmith added inline comments. Comment at: clang/test/Driver/darwin-version.c:217 // RUN: FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s -// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' +// CHECK-VERSION-

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

dexonsmith added inline comments. Comment at: clang/test/Driver/darwin-version.c:217 // RUN: FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s -// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' +// CHECK-VERSION-

[PATCH] D74094: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

dexonsmith added subscribers: arphaman, akyrtzi, jroelofs. dexonsmith added a comment. In D74094#4554327 , @foad wrote: > Hi @erik.pilkington, I see this got reverted: I'm not sure if @erik.pilkington is still watching Phabricator, but in any case I thin

[PATCH] D89001: [clang] Don't look into for C++ headers if they are found alongside the toolchain

dexonsmith added a comment. I don't have access to rdar these days to look into the current state or to refresh my memory. My memory is that the high-level goal was to move the libc++ headers from the toolchain to the SDK. For the transition, this was staged in such a way that they were in bot

[PATCH] D157011: [Clang][Tooling] Accept preprocessed input files

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

[PATCH] D157283: [clang] Match -isysroot behaviour with system compiler on Darwin

dexonsmith resigned from this revision. dexonsmith added subscribers: arphaman, jroelofs. dexonsmith added a comment. This looks correct to me, but I'd rather have someone at Apple confirm. @ldionne (or @arphaman or @jroelofs), can you review and/or help to find the right person? Repository:

[PATCH] D89001: [clang] Don't look into for C++ headers if they are found alongside the toolchain

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

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

dexonsmith added inline comments. Comment at: clang/test/Modules/Inputs/System/usr/include/stdint.h:2 typedef int my_awesome_nonstandard_integer_type; + +/* C99 7.18.1.1 Exact-width integer types. benlangmuir wrote: > iana wrote: > > iana wrote: > > > benlangmui

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

dexonsmith added a comment. In D74094#4645562 , @nickdesaulniers wrote: > I don't yet fully comprehend yet what's going wrong, and probably need to > familiarize myself with the language rules around `auto`'s type deduction. For reduction purposes, it m

[PATCH] D154502: [AST] Fix bug in UnresolvedSet::erase of last element

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

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

dexonsmith added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:542 +N = Decls.size(); + } + john.brawn wrote: > rjmccall wrote: > > john.brawn wrote: > > > rjmccall wrote: > > > > This is going to fire on every single ordinary lookup that finds

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

dexonsmith added a comment. Now that the behaviour change is understood, maybe it'd be useful to split the patch in two: - First, this patch, plus a call to `ASTReader::getInputFile()` only for its side effects, to make this patch actually NFC. - Second, committed a few days later, a patch that

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

dexonsmith added inline comments. Comment at: clang/lib/Headers/__stddef_null.h:14 + * __need_NULL and rely on stddef.h to redefine NULL to the correct value again. + * Modules don't support redefining macros like that, but support that pattern + * in the non-modules case. -

[PATCH] D159064: [Modules] Make clang modules for the C standard library headers

dexonsmith added inline comments. Comment at: clang/lib/Headers/__stddef_null.h:14 + * __need_NULL and rely on stddef.h to redefine NULL to the correct value again. + * Modules don't support redefining macros like that, but support that pattern + * in the non-modules case. -

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

dexonsmith added a subscriber: rnk. dexonsmith added inline comments. Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878 + // with native path seperator, regardless of the actual path seperator + // used in YAMLFilePath field. +#ifndef _WIN32 bnb

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

dexonsmith added a comment. I just realized @jansvoboda11 is probably out on holiday this week (IIRC, Apple usually gets this week off). Since this was committed almost a month ago, I'm guessing this isn't enough of a blocker that we need to revert rather than wait until next week (and there ar

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

dexonsmith added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:6343 "Invalid data, missing pragma diagnostic states"); - SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]); - auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc); -

[PATCH] D137213: [clang][modules] NFCI: Pragma diagnostic mappings: write/read FileID instead of SourceLocation

dexonsmith added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:6343 "Invalid data, missing pragma diagnostic states"); - SourceLocation Loc = ReadSourceLocation(F, Record[Idx++]); - auto IDAndOffset = SourceMgr.getDecomposedLoc(Loc); -

[PATCH] D76916: [Darwin] Respect -fno-unroll-loops during LTO.

dexonsmith added a subscriber: Florian. dexonsmith added a comment. @fhahn, please revert, this isn't how we usually pass options in LTO. If this is something we expect developers to use, it should be specifiable on a per-TU basis. The way we do this is by specifying it during compilation, att

[PATCH] D77058: [Clang] Add llvm.loop.unroll.disable to loops with -fno-unroll-loops.

dexonsmith added a comment. In D77058#1950019 , @tejohnson wrote: > I think this is a good approach, rather than a per-function attribute, since > as mentioned this will be preserved through inlining. > @dexonsmith, does that seem reasonable to you? I mi

[PATCH] D76594: [clang][AST] Support AST files larger than 512M

dexonsmith resigned from this revision. dexonsmith added a reviewer: Bigcheese. dexonsmith added a subscriber: Bigcheese. dexonsmith added a comment. Herald added a subscriber: dexonsmith. In D76594#1957452 , @DmitryPolukhin wrote: > @rsmith, @dexonsmit

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

dexonsmith added a comment. Thanks for working on this, I agree it's a really important problem. I'm as optimistic as Vedant that this is the right approach though. - On compile time, I do think there's reason to be concerned, since dumping IR was fairly expensive last I checked due to numberin

[PATCH] D69471: [Coverage] Revise format to reduce binary size

dexonsmith added a comment. In D69471#1883912 , @rnk wrote: > Everything is off-by-one because the empty bases are not zero sized. The MSVC > record layout algorithm is just different in this area. =/ Do all the MSVCs we support building with support `_

[PATCH] D74939: clang/Modules: Finish renaming CompilerInstance::ModuleManager, NFC.

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Thanks! LGTM. Don't know how I missed that API... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74939/new/ https://reviews.llvm.org/D7

[Diffusion] rG396b7253944e: [OpenMP][Opt] Combine `struct ident_t*` during deduplication

dexonsmith added subscribers: aprantl, cfe-commits, dexonsmith. dexonsmith added inline comments. BRANCHES master /clang/test/OpenMP/PR44893.c:1 Can you change this to a `-cc1` test (invoking `%clang_cc1`), instead of calling the driver? /clang/test/OpenMP/PR44893.c:3 Can you add some sort of

[PATCH] D74784: [driver][darwin] Don't use -platform_version flag by default

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM to me too. With @steven_wu and me on board, I don't think you need to wait for @arphaman. Thanks for the fix! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74784/new/

[PATCH] D75395: [clang][Modules] Add -fsystem-module flag

dexonsmith added a comment. > This is used when > converting an implicit build to an explicit build to match the > systemness the implicit build would have had for a given module. I had another thought. What if for the explicitly built "system" modules: - If `-Wsystem-headers` is on, leave th

[PATCH] D75395: [clang][Modules] Add -fsystem-module flag

dexonsmith added inline comments. Comment at: clang/include/clang/Driver/Options.td:1448 Flags<[CC1Option]>, Alias; +def fsystem_module : Flag<["-"], "fsystem-module">, Flags<[CC1Option]>, + HelpText<"Build this module as a system module. Only used with -emit-module">; -

[PATCH] D75395: [clang][Modules] Add -fsystem-module flag

dexonsmith added a comment. In D75395#1901859 , @Bigcheese wrote: > In D75395#1901778 , @dexonsmith > wrote: > > > > This is used when > > > converting an implicit build to an explicit build to match the > > > sy

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

dexonsmith added inline comments. Comment at: clang/lib/Serialization/ModuleManager.cpp:183 // Get a buffer of the file and close the file descriptor when done. - Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false); + Buf = FileMgr.getBufferForF

[PATCH] D69498: IR: Invert convergent attribute handling

dexonsmith added a comment. In D69498#1723606 , @rjmccall wrote: > Perhaps there should be a global metadata, or something in the > increasingly-misnamed "data layout" string, which says that convergence is > meaningful, and we should only add the attrib

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

dexonsmith abandoned this revision. dexonsmith added a comment. I just pushed 31e14f41a21f9016050a20f07d5da03db2e8c13e , which moves KnownModules into ModuleMap as an alternative. In D58497#1648134

[PATCH] D70056: clang/Modules: Split loop in ReadAST between failable and not

dexonsmith created this revision. dexonsmith added reviewers: aprantl, bruno, Bigcheese. Herald added a subscriber: ributzka. Split a loop in ReadAST that visits the just-loaded module chain, between an initial loop that reads further from the ASTs (and can fail) and a second loop that does some p

[PATCH] D70058: clang/Modules: Delay err_module_file_conflict if a diagnostic is in flight

dexonsmith created this revision. dexonsmith added reviewers: aprantl, bruno, Bigcheese. Herald added a subscriber: ributzka. As part of an audit of whether all errors are being reported from the ASTReader, delay err_module_file_conflict if a diagnostic is already in flight when it is hit. This r

[PATCH] D70055: clang/Modules: Clean up modules on error in ReadAST

dexonsmith created this revision. dexonsmith added reviewers: bruno, aprantl, Bigcheese. Herald added a subscriber: ributzka. ReadASTBlock and ReadASTExtensions can both return failures. Be consistent and remove all the just-loaded modules, just like when ReadASTCore returns failures. https://r

[PATCH] D70058: clang/Modules: Delay err_module_file_conflict if a diagnostic is in flight

dexonsmith updated this revision to Diff 228618. dexonsmith added a comment. Updated header docs for the new delayed diagnostic parameter. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70058/new/ https://reviews.llvm.org/D70058 Files: clang/include/clang/Basic/Diagnostic.h clang/in

[PATCH] D70057: clang/Modules: Add missing diagnostics for malformed AST files

dexonsmith created this revision. dexonsmith added reviewers: aprantl, bruno, Bigcheese. Herald added a subscriber: ributzka. These were found via an audit. In the case of `ParseLineTable` this is actually dead code, since parsing the line table always succeeds, but it's prudent to be defensive s

[PATCH] D70063: clang/Modules: Error if ReadASTBlock does not find the main module

dexonsmith created this revision. dexonsmith added reviewers: aprantl, bruno, Bigcheese. Herald added a subscriber: ributzka. dexonsmith added parent revisions: D70055: clang/Modules: Clean up modules on error in ReadAST, D70056: clang/Modules: Split loop in ReadAST between failable and not, D700

[PATCH] D70063: clang/Modules: Error if ReadASTBlock does not find the main module

dexonsmith updated this revision to Diff 228626. dexonsmith added a comment. Updated to use a new diagnostic (`err_module_file_missing_definition`) that includes the filename of the PCM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70063/new/ https://reviews.llvm.org/D70063 Files:

[PATCH] D70056: clang/Modules: Split loop in ReadAST between failable and not

dexonsmith marked an inline comment as done. dexonsmith added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:4236 + MEnd = Loaded.end(); + M != MEnd; ++M) { +ModuleFile &F = *M->Mod; apran

[PATCH] D70058: clang/Modules: Delay err_module_file_conflict if a diagnostic is in flight

dexonsmith closed this revision. dexonsmith added a comment. Committed as eef69021607950487a9e4110851a05abb54d0fb6 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70058/new/ https://reviews.llvm.org/D70058

[PATCH] D70056: clang/Modules: Split loop in ReadAST between failable and not

dexonsmith closed this revision. dexonsmith added a comment. Committed in bfd58fc60ff4b0c081b5b489119c3798d3e2b53c and 01782c3e4df1830d7991e9edfee9119ed41e4c27

[PATCH] D70057: clang/Modules: Add missing diagnostics for malformed AST files

dexonsmith closed this revision. dexonsmith added a comment. Committed 8e2c192e2af8c760152ba3b28e774dbb1548e4aa . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70057/new/ https://reviews.llvm.org/D70057 ___

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

dexonsmith added a comment. In D69959#1741456 , @rnk wrote: > What do you think of making `FrontendOptions::Inputs` be a `SmallVector<*, > 0>`? It's janky and inconsistent with the rest of that struct, but it seems > less invasive in the end. +1, we us

[PATCH] D70055: clang/Modules: Clean up modules on error in ReadAST

dexonsmith closed this revision. dexonsmith added a comment. Committed c46b3a2abd38d6fecd389c97dfa7df54af77fdb9 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70055/new/ https://reviews.llvm.org/D70055 ___

[PATCH] D70063: clang/Modules: Error if ReadASTBlock does not find the main module

dexonsmith marked 2 inline comments as done. dexonsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSerializationKinds.td:78 +def err_module_file_missing_definition : Error< + "module file '%0' is missing the main module's definition">, DefaultFatal;

[PATCH] D70063: clang/Modules: Error if ReadASTBlock does not find the main module

dexonsmith closed this revision. dexonsmith marked 2 inline comments as done. dexonsmith added a comment. Committed 83dcb34b6bf4c175040b18d3e8c3c468418009fc . Thanks for the reviews! Comment at: clang/lib/S

[PATCH] D69841: Target Ivy bridge on macOS Mojave and later

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. Hi Dave, thanks for checking in on this, but unfortunately we're not ready for this yet. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

dexonsmith added inline comments. Comment at: clang/test/CodeGenObjC/direct-method.m:25 + // CHECK-NEXT: store %0* %self, %0** %self.addr, + // CHECK-NEXT: store i8* %_cmd, i8** %_cmd.addr, + rjmccall wrote: > The IR names of local values aren't emitted in rele

[PATCH] D70285: Wrap C APIs with pragmas enforcing -Werror=strict-prototypes

dexonsmith created this revision. dexonsmith added reviewers: aaron.ballman, Bigcheese, jkorous-apple. Herald added subscribers: ributzka, arphaman, steven_wu, hiraditya, mehdi_amini. Herald added a reviewer: deadalnix. Herald added a project: LLVM. Force `-Werror=strict-prototypes` so that C API

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

dexonsmith added a comment. The code looks right, but can you add a test for this in CodeGenObjC? Also, when you re-upload the patch with the tests, please use `-U999` to provide full context in the diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D70285: Wrap C APIs with pragmas enforcing -Werror=strict-prototypes

dexonsmith added a comment. Note that we have C files including these headers in llvm/tools/llvm-c-test and clang/tools/c-index-test, and I tested that dropping a `(void)` to just `()` triggers the error when compiling those. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70285/new/ ht

[PATCH] D70285: Wrap C APIs with pragmas enforcing -Werror=strict-prototypes

dexonsmith updated this revision to Diff 229621. dexonsmith added a comment. Adding guards for `_MSC_VER`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70285/new/ https://reviews.llvm.org/D70285 Files: clang/include/clang-c/BuildSystem.h clang/include/clang-c/CXCompilationDatabase

[PATCH] D70285: Wrap C APIs with pragmas enforcing -Werror=strict-prototypes

dexonsmith marked 2 inline comments as done. dexonsmith added inline comments. Comment at: clang/include/clang-c/ExternC.h:18-19 +#define LLVM_CLANG_C_STRING_PROTOTYPES_BEGIN \ + _Pragma("clang diagnostic push")

[PATCH] D70285: Wrap C APIs with pragmas enforcing -Werror=strict-prototypes

dexonsmith updated this revision to Diff 229624. dexonsmith marked an inline comment as done. dexonsmith added a comment. s/C_STRING_PROTOTYPES_/C_STRICT_PROTOTYPES_/ to fix some typos. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70285/new/ https://reviews.llvm.org/D70285 Files: cl

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

dexonsmith closed this revision. dexonsmith added a comment. Pushed as d4e1ba3fa9dfec2613bdcc7db0b58dea490c56b1 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69991/new/ https://reviews.llvm.org/D69991 ___

[PATCH] D70285: Wrap C APIs with pragmas enforcing -Werror=strict-prototypes

dexonsmith added a comment. In D70285#1752164 , @Bigcheese wrote: > lgtm as long as other compilers don't warn on unknown pragmas by default. godbolt.org says that GCC doesn't have `-Wunknown-pragmas` by default, but it is in `-Wall`. I'd better guard

[PATCH] D70285: Wrap C APIs with pragmas enforcing -Werror=strict-prototypes

dexonsmith closed this revision. dexonsmith added a comment. In D70285#1752361 , @dexonsmith wrote: > In D70285#1752164 , @Bigcheese wrote: > > > lgtm as long as other compilers don't warn on unknown pragmas by defa

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

dexonsmith added a comment. For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer? The two comments: - Please add a period at the end of the sentence in the comment. - Can you give more context

[PATCH] D70556: clang/Modules: Refactor CompilerInstance::loadModule, NFC

dexonsmith created this revision. dexonsmith added reviewers: bruno, Bigcheese, jkorous, rsmith. Herald added a subscriber: ributzka. Refactor the logic on CompilerInstance::loadModule and a couple of surrounding methods in order to clarify what's going on. - Rename ModuleLoader::loadModuleFromSo

[PATCH] D70556: clang/Modules: Refactor CompilerInstance::loadModule, NFC

dexonsmith marked 2 inline comments as done. dexonsmith added inline comments. Comment at: clang/include/clang/Frontend/CompilerInstance.h:801-804 + ModuleLoadResult findOrCompileModuleAndReadAST(StringRef ModuleName, + SourceLocat

[PATCH] D70556: clang/Modules: Refactor CompilerInstance::loadModule, NFC

dexonsmith marked 2 inline comments as done. dexonsmith added inline comments. Comment at: clang/include/clang/Lex/ModuleLoader.h:50 +// We failed to load the module, but we shouldn't cache the failure. +OtherUncachedFailure, }; jkorous wrote: > Just a

[PATCH] D70583: clang/Modules: Rename CompilerInstance::ModuleManager, NFC

dexonsmith created this revision. dexonsmith added reviewers: Bigcheese, jkorous, rsmith, bruno. Herald added subscribers: ributzka, kbarton, nemanjai. dexonsmith added a parent revision: D70556: clang/Modules: Refactor CompilerInstance::loadModule, NFC. Herald added a subscriber: wuzish. Fix the

[PATCH] D70556: clang/Modules: Refactor CompilerInstance::loadModule, NFC

dexonsmith closed this revision. dexonsmith marked an inline comment as done. dexonsmith added a comment. Pushed as 20d51b2f14ac4488f684f8fc57cb0ba718a6b91d. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70556/new/ https://reviews.llvm.org/D70556 _

[PATCH] D70583: clang/Modules: Rename CompilerInstance::ModuleManager, NFC

dexonsmith closed this revision. dexonsmith added a comment. Pushed as 20d51b2f14ac4488f684f8fc57cb0ba718a6b91d . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70583/new/ https://reviews.llvm.org/D70583 ___

[PATCH] D70936: [clang-scan-deps] do not skip empty #if/#elif in the minimize to avoid missing `__has_include` dependencies

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, although I have a suggested change for one of the comments (and I'm still sad about this). Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:76

[PATCH] D70568: [Support] Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions

dexonsmith added a comment. This is really cool; we've wanted this for a long time. One concern I have is that I think this will interfere (effectively disable) automatic OS handling of these crashes, which means they won't be collated and reported anymore. Can we make this configurable someho

[PATCH] D70568: [Support] Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions

dexonsmith added a comment. In D70568#1768129 , @dexonsmith wrote: > Can we make this configurable somehow? (E.g., leave behind an `-ffork-cc1` > `-fno-fork-cc1` and a CMake flag to pick? Or just the CMake flag?) (I'm not sure we'd want to expose this

[PATCH] D70568: [Support] Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions

dexonsmith added a comment. In D70568#1768138 , @dexonsmith wrote: > In D70568#1768129 , @dexonsmith > wrote: > > > > Sorry, you can ignore all of my high-level comments, I thought I was commenting on https://re

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

dexonsmith added a comment. In D69825#1771300 , @arphaman wrote: > I guess Duncan's comments on https://reviews.llvm.org/D70568 were for this > patch. I think it would be good to have: That's right, I was confused about which review I was on. > - A cma

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

dexonsmith added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2077 +getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy))) + SRETAttrs.addAlignmentAttr(Align); ArgAttrs[IRFunctionArgs.getSRetArgNo()] = scanon wrote: > r

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

dexonsmith added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2077 +getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy))) + SRETAttrs.addAlignmentAttr(Align); ArgAttrs[IRFunctionArgs.getSRetArgNo()] = rjmccall wrote: >

[PATCH] D74784: [driver][darwin] Don't use -platform_version flag by default

dexonsmith added a comment. Code change looks correct to me. Thanks for the fix! @arphaman, can you confirm the test changes are reasonable? My instinct would have been, instead of changing all of the 400s to 0s, to just adding a single `RUN` line somewhere to confirm we don't do the wrong th

[PATCH] D74795: Make diagnostic reporting more robust in presence of corrupt PCH data.

dexonsmith added a comment. Thanks for working on this! I have a couple of comments inline. Comment at: clang/include/clang/Basic/Diagnostic.h:918-927 /// The ID of the current diagnostic that is in flight. /// /// This is set to std::numeric_limits::max() when there

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/Serialization/ModuleManager.cpp:183 // Get a buffer of the file and close the file descriptor when done. - Buf = FileMgr.getBu

[PATCH] D72970: clang: Only define OBJC_NEW_PROPERTIES when -x objective-c

dexonsmith created this revision. dexonsmith added reviewers: rjmccall, arphaman. Herald added a subscriber: ributzka. Since 2009 (in r63846) we've been `#define`-ing OBJC_NEW_PROPERTIES all the time on Darwin, but this macro only makes sense for `-x objective-c` and `-x objective-c++`. Restrict

[PATCH] D73219: [objc_direct] do not add direct properties to the serialization array

dexonsmith added inline comments. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:3316-3319 else if (const ObjCCategoryDecl *CD = dyn_cast(OCD)) { for (const auto *P : CD->protocols()) PushProtocolProperties(PropertySet, Properties, P, IsClassProperty); } --

[PATCH] D73208: [objc_direct] fix codegen for mismatched Decl/Impl return types

dexonsmith added a comment. Why isn't a similar dance needed for non-direct methods? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73208/new/ https://reviews.llvm.org/D73208 ___ cfe-commits mailing lis

[PATCH] D73219: [objc_direct] do not add direct properties to the serialization array

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

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

dexonsmith added a comment. In D67678#1836628 , @steven_wu wrote: > In D67678#1834957 , @rsmith wrote: > > > In D67678#1834542 , @steven_wu > > wrote: > > > > > @rsmith Th

[PATCH] D73208: [objc_direct] fix codegen for mismatched Decl/Impl return types

dexonsmith added a comment. In D73208#1835264 , @MadCoder wrote: > In D73208#1835051 , @dexonsmith > wrote: > > > Why isn't a similar dance needed for non-direct methods? > > > because non direct methods do not nee

[PATCH] D73208: [objc_direct] fix codegen for mismatched Decl/Impl return types

dexonsmith added a comment. In D73208#1836722 , @MadCoder wrote: > In D73208#1836704 , @dexonsmith > wrote: > > > In D73208#1835264 , @MadCoder > > wrote: > > > > > In D732

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

dexonsmith added a comment. In D67678#1836922 , @rsmith wrote: > In D67678#1836668 , @dexonsmith > wrote: > > > In D67678#1836628 , @steven_wu > > wrote: > > > > > In D6767

<    6   7   8   9   10   11   12   13   14   15   >