[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. 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] 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] 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] 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] 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] 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] 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. 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. 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: 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] 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] 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] 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] 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] 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] 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] 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] 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] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

dexonsmith added subscribers: iana, ributzka. dexonsmith added a comment. In D103930#4310061 , @ivanmurashko wrote: > Friendly ping > > @arphaman, @jansvoboda11, I have made the patch buildable on all platforms > and have all tests passed. There was als

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. In D83906#4186887 , @hoy wrote: > In D83906#4184916 , @dexonsmith > wrote: > >> In D83906#4183453 , @hoy wrote: >> >>> Wondering if we can come u

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. In D83906#4183453 , @hoy wrote: > Wondering if we can come up with a way to tell the optimizer about that, > e.g., through a new module flag. When it comes to LTO, the selection of > linkonce_odr symbols should already been do

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a subscriber: sanjoy. dexonsmith added a comment. In D83906#4182902 , @rjmccall wrote: > So your argument is that it would not be possible to recognize that we're > doing such an optimization and mark the function as having had a possible

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. In D83906#4182847 , @hoy wrote: > As far as I know, the optimizer IPO pass that infers function attributes > (i..e `InferFunctionAttrsPass`) is placed at the very beginning of the > optimization pipeline. Does this sound to y

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. In D83906#4182777 , @rjmccall wrote: > In D83906#4182287 , @dexonsmith > wrote: > >> - At IRGen time, you know the LLVM attributes have not been adjusted after >> the optimized refined

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. In D83906#4182428 , @hoy wrote: > In D83906#4182287 , @dexonsmith > wrote: > >> In C++, you get linkonce_odr all over the place. It's basically all >> functions that are defined in C++

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. In D83906#4181981 , @hoy wrote: > That said, the LLVM optimizer does not strictly subsume the front-end because > of how it fails to handle `linkonce_odr` functions as in > https://reviews.llvm.org/D18634. I'm wondering how co

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

dexonsmith added inline comments. Comment at: clang/tools/libclang/CXType.cpp:13 +#include "CXType.h" #include "CIndexer.h" vedgy wrote: > collinbaker wrote: > > vedgy wrote: > > > I guess //clang-format// did this include reordering. But it certainly > > > l

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. Oh, de-refining is pretty nifty / evil. This patch has background: https://reviews.llvm.org/D18634 Since 2016, the optimizer is not allowed to do IPA on functions that can be de-refined (such as `linkonce_odr` functions). Here's a hypothetical problematic scenario fo

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

dexonsmith added a comment. In D83906#4179512 , @wlei wrote: > Hi @ahatanak > > We recently hit an issue of inconsistent codegen related with this > optimization. In one build, Clang frontend generates different llvm IRs for > the same function that is o

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

dexonsmith added a comment. In D130303#4175664 , @collinbaker wrote: > @dexonsmith can you weigh in? Introducing `clang_isBitFieldDecl` sounds like a clean/straightforward solution to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. In D130303#3724392 , @dexonsmith wrote: > In D130303#3724247 , @rnk wrote: > >> Pinging alternative

[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] 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] 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] 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] 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: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: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: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 a comment. > Consider making the FileEntryRef changes here the default -- it doesn't make > sense to me that FileEntryRef == or DenseMap would match FileEntry pointer > semantics instead of filename semantics. Yeah, that was something I added, and I agree it's unfortunate / har

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with one suggestion for the test inline. Comment at: clang/test/Modules/add-remove-irrelevant-module-map.m:22 +// Build modules with the non-affecting "a/module

[PATCH] D137216: [clang][modules] NFCI: Avoid unnecessary serialization logic for non-affecting 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/D137216/new/ https://reviews.llvm.org/D137216 __

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

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, although I there's format-is-probably-compatible-version as a courtesy for tooling, does that need a bump here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D137211: [clang][modules] NFCI: Unify FileID writing/reading

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/D137211/new/ https://reviews.llvm.org/D137211 __

[PATCH] D137214: [clang][modules] NFCI: Scaffolding for serialization of adjusted SourceManager offsets

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/D137214/new/ https://reviews.llvm.org/D137214 __

[PATCH] D137215: [clang] NFC: Extract lower-level SourceManager functions

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/D137215/new/ https://reviews.llvm.org/D137215 __

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3900593 , @jansvoboda11 wrote: > Ah, I forgot to mention this. Building the modules is now only 0.2% slower > and importing them 1.2% faster (compared to PCMs with all input files > serialized). Awesome. All upsi

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3900051 , @jansvoboda11 wrote: >> - Is there a change in cycles/instructions when the module cache is hot? >> (presumably the common case) > > I didn't notice this (but didn't look for it specifically). How could t

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3898849 , @hans wrote: > Relatedly, we ran into a problem with `clang-cl /showIncludes` not including > all files in its output when they're linked: > https://github.com/llvm/llvm-project/issues/58726 Interestingl

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3870991 , @hans wrote: > One thought, which I'm not sure is relevant, is that this is only observable > for headers which are included more than once, which is rare because normally > there are include guards (or p

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3893001 , @jansvoboda11 wrote: > I tried implementing your suggestion (merging ranges of adjacent > non-affecting files and avoiding `FileID` lookups), but the numbers from > `-ftime-trace` are very noisy. I got m

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added inline comments. Comment at: clang/test/Modules/non-affecting-module-maps-source-locations.m:32 + +// RUN: %clang_cc1 -I %t/first -I %t/second -I %t/third -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %t/tu.m -o %t/tu.o dexonsmi

[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

dexonsmith added a comment. In D136624#3888387 , @jansvoboda11 wrote: > I tried optimizing this patch a bit. Instead of creating compact data > structure and using binary search to find the preceding non-affecting file, I > now store the adjustment inf

[PATCH] D106876: Remove non-affecting module maps from PCM files.

dexonsmith added a comment. In D106876#3869447 , @jansvoboda11 wrote: > I agree that avoiding serializing non-affecting input files is the better > approach. Your code is more or less what I had in mind, thanks for sketching > it out! > The number of i

[PATCH] D106876: Remove non-affecting module maps from PCM files.

dexonsmith added a comment. In D106876#3869247 , @dexonsmith wrote: > A further (more involved) approach would be to separate module maps into a > separate SourceManager, so that their source locations don't affect other > input files. Then only module

[PATCH] D106876: Remove non-affecting module maps from PCM files.

dexonsmith added a comment. As an end goal, it seems strictly better for build systems / distribution / artifact storage / caching if the output PCM has been canonicalized as-if the module maps weren't found, rather than tracking both which module maps were discovered and which to ignore. Ideal

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3862058 , @hans wrote: > The build system folks replied saying they're not using symlinks, but hard > links for compiler inputs. Dropping the `-s` flag in the repro above > demonstrates this. I think this only hap

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

dexonsmith added a comment. I would think we could convert every assert(0) to either `llvm::report_fatal_error` (guaranteed trap) or `llvm_unreachable()` (trap or optimize, depending on CMAKE configuration). The C API usage checks seem like good candidates for the former. Also, not sure if eve

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3840257 , @goncharov wrote: > It's possbile to make it deterministic by making headers unique though. Also, this is the first time you've mentioned non-determinism. Is this non-deterministic? Repository: rG LLV

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3840257 , @goncharov wrote: > In D135220#3840177 , @dexonsmith > wrote: > >> In D135220#3839671 , @goncharov >> wrote: >> >>> That

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

dexonsmith added a comment. In D135220#3839671 , @goncharov wrote: > That change might be problematic for content addressing storages. E.g. > clang/test/Driver/cl-pch-showincludes.cpp started to fail in my setup as > clang/test/Driver/header{0,1,3,4}.h

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

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

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827478 , @aaron.ballman wrote: > In D134456#3827410 , @dexonsmith > wrote: > >> In D134456#3827377 , >> @aaron.ballman wrote: >>

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827377 , @aaron.ballman wrote: > In D134456#3827332 , @dexonsmith > wrote: > >> In D134456#3827263 , >> @aaron.ballman wrote: >>

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827332 , @dexonsmith wrote: > In D134456#3827263 , @aaron.ballman > wrote: > >> My worry is that parallel attributes will be used as: >> >> #if __has_cpp_attribute(clang

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827263 , @aaron.ballman wrote: > My worry is that parallel attributes will be used as: > > #if __has_cpp_attribute(clang::likely_but_honor_this_one) > #define LIKELY [[clang::likely_but_honor_this_one]] > #el

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827263 , @aaron.ballman wrote: > In D134456#3827185 , @dexonsmith > wrote: > >> This safety scenario sounds like it could differ within a file. Is a flag >> really the ri

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

dexonsmith added a comment. In D134456#3827040 , @aaron.ballman wrote: > In D134456#3819185 , @rnk wrote: > >> In D134456#3819042 , >> @aaron.ballman wrote: >> >>> Alter

[PATCH] D95502: WIP: Frontend: Adopt llvm::vfs::OutputBackend in CompilerInstance

dexonsmith abandoned this revision. dexonsmith added a comment. Herald added a project: All. This is superseded by https://reviews.llvm.org/D133509. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95502/new/ https://reviews.llvm.org/D95502 ___

[PATCH] D132568: [clang][Sema] check default argument promotions for printf

dexonsmith added a subscriber: arphaman. dexonsmith added inline comments. Comment at: clang/test/FixIt/format.m:195-208 - NSLog(@"%C", 0x260300); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'int'}} - // CHECK-NOT: fix

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

dexonsmith added a comment. In D130303#3724247 , @rnk wrote: > Pinging alternative reviewer +@dexonsmith for a libclang API addition Looks reasonable to me -- this only changes behaviour of the existing API when there was corruption before -- but if the

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

dexonsmith added inline comments. Comment at: llvm/include/llvm/ADT/SCCIterator.h:165-170 + SCCProxy operator*() const { assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!"); return CurrentSCC; } + SCCProxy operator->() const { return operator*(); } -

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

dexonsmith added a comment. In D126676#3687491 , @mstorsjo wrote: > In D126676#3687227 , @dexonsmith > wrote: > >> There are cases where it’s safe to have mismatched defines (e.g., if a >> define can be proven d

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

dexonsmith added a comment. I haven’t reviewed the details of the patch and won’t have time to do so (at least for a while), but the description of the intended (more narrow) scope SGTM. With the scope limited to GCC directories this should be fine. There are cases where it’s safe to have misma

[PATCH] D129446: [clang][driver] Find Apple default SDK path

dexonsmith added a subscriber: arphaman. dexonsmith added a comment. I’m not at Apple anymore, but this is a long-standing platform decision that is intentional and seems unlikely to change. Note that `/usr/bin/clang` isn’t really clang, but a tool called `xcrun` which adds environment variables

[PATCH] D129226: [clang/mac] Make -mmacos-version-min the canonical spelling over -mmacosx-version-min

dexonsmith added a reviewer: akyrtzi. dexonsmith added a subscriber: akyrtzi. dexonsmith added a comment. LGTM too, but I’m not at Apple these days. @akyrtzi, can you help find someone appropriate to take a quick look since @arphaman seems busy? CHANGES SINCE LAST ACTION https://reviews.llvm

[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] 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] 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] D125487: [Tooling/DependencyScanning] Refactor dependency scanning to produce pre-lexed preprocessor directive tokens, instead of minimized sources

dexonsmith added inline comments. Comment at: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp:175 "#define MACRO con tent ", Out)); - EXPECT_STREQ("#define MACRO con tent\n", Out.data()); + EXPECT_STREQ("#define MACRO con tent\n", Out.data()); ---

[PATCH] D125487: [Tooling/DependencyScanning] Refactor dependency scanning to produce pre-lexed preprocessor directive tokens, instead of minimized sources

dexonsmith added inline comments. Comment at: clang/unittests/Lex/DependencyDirectivesScannerTest.cpp:175 "#define MACRO con tent ", Out)); - EXPECT_STREQ("#define MACRO con tent\n", Out.data()); + EXPECT_STREQ("#define MACRO con tent\n", Out.data()); ---

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added a comment. In D122895#3511855 , @aaron.ballman wrote: >> (It also seems unfortunate to regress the false positive rate of this >> diagnostic before `-fstrict-prototypes` is available.) > > `-fno-knr-functions` is available already today

[PATCH] D124974: [clang] Include clang config.h in LangStandards.cpp

dexonsmith accepted this revision. dexonsmith added a comment. LGTM, thanks! One comment online below. Comment at: clang/include/clang/Config/config.h.cmake:19 #cmakedefine CLANG_DEFAULT_STD_C LangStandard::lang_${CLANG_DEFAULT_STD_C} +// Always #define something so that missi

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added a comment. In D122895#3511649 , @aaron.ballman wrote: > In D122895#3511611 , @dexonsmith > wrote: > >> Sure, I'm all for adding a new warning for users that want a pedantic >> warning. Can it b

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added a comment. In D122895#3511611 , @dexonsmith wrote: > Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in > code. For example, it's important to have a warning that catches code like this: void f1(void (^block)

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added a subscriber: arphaman. dexonsmith added a comment. In D122895#3511376 , @aaron.ballman wrote: > In D122895#3511312 , @aaron.ballman > wrote: > >> However, I think the blocks behavior is a bug b

[PATCH] D125488: [Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`

dexonsmith added a comment. In D125488#3510320 , @akyrtzi wrote: > In D125488#3510297 , @dexonsmith > wrote: > >> [To be clear, my question was because I don't see this patch deleting the >> code path that minim

[PATCH] D125488: [Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`

dexonsmith added a comment. In D125488#3510265 , @akyrtzi wrote: > In D125488#3510214 , @dexonsmith > wrote: > >> Is there code in DepFS that can/should be deleted as part of this patch, or >> in a follow-up, or

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added inline comments. Comment at: clang/test/Sema/warn-strict-prototypes.m:20 + // know it's a block when diagnosing. + void (^block2)(void) = ^void() { // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}} };

[PATCH] D125488: [Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`

dexonsmith added a comment. Awesome to see this! Looking forward to the next step (using this in normal preprocessing!). > after this change, we don't minimize sources and pass them in place of the > real sources Is there code in DepFS that can/should be deleted as part of this patch, or in a

[PATCH] D124974: [clang] Include clang config.h in LangStandards.cpp

dexonsmith added a comment. In D124974#3504986 , @porglezomp wrote: > Ah, so it'd be a test that passes pretty trivially on any bot that doesn't > have a custom version of `CLANG_DEFAULT_STD_C` and `CLANG_DEFAULT_STD_CXX` > like the default config, but

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added a comment. For additional context to my questions above, even though open source clang hasn't been using `-Wstrict-prototypes`, Xcode has had it on-by-default in new projects since sometime in 2017, with project modernizations to turn it on for old projects. Warning on block a

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added inline comments. Comment at: clang/test/Sema/warn-strict-prototypes.m:20 + // know it's a block when diagnosing. + void (^block2)(void) = ^void() { // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}} };

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added subscribers: steven_wu, rjmccall, rnk, dexonsmith. dexonsmith added inline comments. Comment at: clang/test/Sema/warn-strict-prototypes.m:20 + // know it's a block when diagnosing. + void (^block2)(void) = ^void() { // expected-warning {{a function declaration

  1   2   3   4   5   6   7   8   9   10   >