[PATCH] D50719: [libc++] Fix incorrect definition of TEST_HAS_C11_FEATURES

dexonsmith accepted this revision. dexonsmith added a comment. In https://reviews.llvm.org/D50719#1199427, @mclow.lists wrote: > I have pissed in this area, too - See > https://bugs.llvm.org/show_bug.cgi?id=38495 and the proposed resolution here: > https://bugs.llvm.org/attachment.cgi?id=20692

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

dexonsmith added inline comments. Comment at: libcxx/include/__config:798-804 #ifndef _LIBCPP_HIDE_FROM_ABI -# define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDEN _LIBCPP_INTERNAL_LINKAGE +# ifdef _LIBCPP_ABI_HIDDEN_USE_INTERNAL_LINKAGE +#define _LIBCPP_HIDE_FROM_ABI _LIBCPP_HIDDE

[PATCH] D50652: [libcxx] By default, do not use internal_linkage to hide symbols from the ABI

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM if thakis is happy. Repository: rCXX libc++ https://reviews.llvm.org/D50652 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D46694: [diagtool] Install diagtool

dexonsmith added a comment. This SGTM, but I wouldn't mind hearing from others. I wonder if this is worth a quick RFC on cfe-dev? Repository: rC Clang https://reviews.llvm.org/D46694 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. The code looks correct to me, although I have a few suggestions inline. When you resubmit, please include full context (e.g., `git diff -U99`). Jan and I discussed this

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

dexonsmith added a comment. (Somehow I missed Richard's comment when I replied last night, even though it preceded mine by 5 hours...) In https://reviews.llvm.org/D46834#1098727, @rsmith wrote: > This would violate our guidelines for diagnostic messages; see > https://clang.llvm.org/docs/Inter

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

dexonsmith added a reviewer: vsapsai. dexonsmith added a comment. In https://reviews.llvm.org/D17741#1067816, @weimingz wrote: > split the original into two parts. This one supports > -ffile-macro-prefix-to-remove function. > > I locally verified it. To add a test case, do we need to use VFS?

[PATCH] D40864: [Darwin] Add a new -mstack-probe option and enable by default

dexonsmith added a comment. Did this eventually go in? Repository: rC Clang https://reviews.llvm.org/D40864 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

dexonsmith added a comment. In https://reviews.llvm.org/D46834#1102391, @rsmith wrote: > In https://reviews.llvm.org/D46834#1101586, @jkorous wrote: > > > We reconsidered this in light of the policy - thanks for pointing that out > > Richard! > > Just to be sure that I understand it - the poli

[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

dexonsmith added a comment. In https://reviews.llvm.org/D46834#1102407, @rsmith wrote: > In https://reviews.llvm.org/D46834#1102395, @dexonsmith wrote: > > > In https://reviews.llvm.org/D46834#1102391, @rsmith wrote: > > > > > The policy certainly seems designed around the CLI use case. For > >

[PATCH] D46485: Add python tool to dump and construct header maps

dexonsmith added inline comments. Comment at: test/Preprocessor/headermap-rel.c:2-3 // This uses a headermap with this entry: // Foo.h -> Foo/Foo.h Should we delete this comment now that the header map is human-readable? Comment at: tes

[PATCH] D46485: Add python tool to dump and construct header maps

dexonsmith added a comment. Removing the binary header map files is a clear win, but I'd like someone else to approve this. Comment at: test/Preprocessor/headermap-rel2.c:1-2 // This uses a headermap with this entry: // someheader.h -> Product/someheader.h --

[PATCH] D47157: Warning for framework headers using double quote includes

dexonsmith added inline comments. Comment at: lib/Lex/HeaderSearch.cpp:753-754 + IncluderAndDir.second->getName())) +Diags.Report(IncludeLoc, + diag::warn_quoted_include_in_framework_header) +<< Filename; --

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

dexonsmith added a reviewer: vsk. dexonsmith added a comment. In https://reviews.llvm.org/D51240#1213179, @davidxl wrote: > In particular, I don't see much need to do this for instrumentation based PGO > [...] Why not? https://reviews.llvm.org/D51240 __

[PATCH] D51240: Add a flag to remap manglings when reading profile data information.

dexonsmith added a comment. In https://reviews.llvm.org/D51240#1213358, @davidxl wrote: > Re "Why not": > > The common use model of instrumentation based PGO and SamplePGO are quite > different. The former usually uses 'fresh' profile that matches the source. > If there are massive code refact

[PATCH] D30009: Add support for '#pragma clang attribute'

dexonsmith added a subscriber: erik.pilkington. dexonsmith added a comment. In https://reviews.llvm.org/D30009#1218647, @rsmith wrote: > One consequence of this patch (see https://reviews.llvm.org/rL341002) is that > adding documentation to an existing attribute //changes the behavior of > Clan

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

dexonsmith added inline comments. Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:50 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum) +// CHECK-NEXT: ExtVectorType (SubjectMatchRule_type_alias) // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule

[PATCH] D51507: Allow all supportable attributes to be used with #pragma clang attribute.

dexonsmith added inline comments. Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:50 // CHECK-NEXT: EnumExtensibility (SubjectMatchRule_enum) +// CHECK-NEXT: ExtVectorType (SubjectMatchRule_type_alias) // CHECK-NEXT: ExternalSourceSymbol ((SubjectMatchRule

[PATCH] D51440: [ToolChains] Link to compiler-rt with -L + -l when possible

dexonsmith added a comment. In https://reviews.llvm.org/D51440#1225318, @steven_wu wrote: > I do prefer the current approach especially on Darwin. Some concerns of > switching to use "-L + -l" are: > > 1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from > resource-dir and pa

[PATCH] D51789: [WIP][clang] Add the no_extern_template attribute

dexonsmith edited subscribers, added: cfe-commits; removed: llvm-commits. dexonsmith added a comment. +cfe-commits -llvm-commits Repository: rL LLVM https://reviews.llvm.org/D51789 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

[PATCH] D49808: [libc++] Factor duplicate code into function templates

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Nice cleanup! LGTM. Repository: rCXX libc++ https://reviews.llvm.org/D49808 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D49914: [libc++] Add the _LIBCPP_HIDE_FROM_ABI_AFTER_V1 macro

dexonsmith added a comment. This looks correct to me, but I wouldn't mind having someone else take a look too. Comment at: libcxx/include/__config:798 -// Just so we can migrate to _LIBCPP_HIDE_FROM_ABI gradually. -#define _LIBCPP_INLINE_VISIBILITY _LIBCPP_HIDE_FROM_ABI - -#

[PATCH] D33368: [libcxxabi][demangler] Fix a crash in the demangler

dexonsmith added inline comments. Comment at: src/cxa_demangle.cpp:3036 break; -if (db.names.size() < 2) +assert(k0 <= k1 && "parse_type() mutated the name stack"); +if (k1 == k0) ---

[PATCH] D28404: IRGen: Add optnone attribute on function during O0

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Actually, looking through the comments, it appears that everyone (eventually) agreed with the approach in the patch. I agree too. LGTM. Mehdi, are you able to rebase and commit, or s

[PATCH] D33739: [Sema] Improve -Wstrict-prototypes diagnostic message for blocks

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

[PATCH] D33977: [libcxx][WIP] Experimental support for a scheduler for use in the parallel stl algorithms

dexonsmith added a comment. You mention that the lock-free `deque` gives a speedup. I agree that should be left for a follow-up, but what kind of speedup did it give? Have you measured whether the parallel algorithm gives a speedup, compared to a non-parallel algorithm? https://reviews.llvm.

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

dexonsmith added a comment. This is the right idea, although it only covers macOS. Any ideas for how to test this? Comment at: src/experimental/filesystem/operations.cpp:23-28 +// We can use the presence of UTIME_OMIT to detect platforms that do not +// provide utimensat, with

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

dexonsmith added inline comments. Comment at: src/experimental/filesystem/operations.cpp:23-28 +// We can use the presence of UTIME_OMIT to detect platforms that do not +// provide utimensat, with some exceptions on OS X. +#if !defined(UTIME_OMIT) || \ + (defined(__MAC_OS_X_VER

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

dexonsmith added inline comments. Comment at: src/experimental/filesystem/operations.cpp:23-28 +// We can use the presence of UTIME_OMIT to detect platforms that do not +// provide utimensat, with some exceptions on OS X. +#if !defined(UTIME_OMIT) || \ + (defined(__MAC_OS_X_VER

[PATCH] D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple

dexonsmith added inline comments. Comment at: src/experimental/filesystem/operations.cpp:22-24 +#if defined(__APPLE__) +#include +#endif EricWF wrote: > dexonsmith wrote: > > I only just noticed you were including Availability.h. That shouldn't be > > necessar

[PATCH] D34175: [driver][macOS] Pick the system version for the deployment target if the SDK is newer than the system

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

[PATCH] D33177: any: Add availability for experimental::bad_any_cast

dexonsmith closed this revision. dexonsmith added a comment. Committed in r305647. https://reviews.llvm.org/D33177 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34331: func.wrap.func.con: Unset function before destroying anything

dexonsmith created this revision. Be defensive against a reentrant std::function::operator=(), in case the held function object has a non-trivial destructor. Destroying the function object in-place can lead to the destructor being called twice. rdar://problem/32836603 https://reviews.llvm.or

[PATCH] D34332: path: Use string_view_t consistently

dexonsmith created this revision. Most of filesystem/path.cpp uses `string_view_t`. This fixes the two spots that use `string_view` directly. https://reviews.llvm.org/D34332 Files: libcxx/src/experimental/filesystem/path.cpp Index: libcxx/src/experimental/filesystem/path.cpp =

[PATCH] D34332: path: Use string_view_t consistently

dexonsmith closed this revision. dexonsmith added a comment. Fixed in r305661. https://reviews.llvm.org/D34332 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34251: Add a new driver option to disable warning about c++17's non-throwing exception specification in function signature

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. I think `-Wc++1z-mangling` is best. IMO, with C++1z in the name, it's already clear that this is about compatibility. I also don't think we need to get as specific as `-Wc++1z-compat

[PATCH] D34251: Add a new driver option to disable warning about c++17's non-throwing exception specification in function signature

dexonsmith added a comment. > We use the `-Wc++NN-compat-` prefix on all the other subwarnings of > `-Wc++NN-compat` warnings Interesting; I thought I saw a counter-example recently, and took that as the rule (although I can't for the life of me remember what it was). `-Wc++NN-compat-mangling

[PATCH] D34264: Introduce -Wunguarded-availability-new, which is like -Wunguarded-availability, except that it's enabled by default for new deployment targets

dexonsmith added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:7031 +Introduced) && +!S.Diags.isIgnored(diag::warn_unguarded_availability_new, Loc); +diag = NewWarning ? diag::warn_partial_availability_new arphaman wrote: > erik.p

[PATCH] D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute

dexonsmith added a comment. > I followed the other availability macros in using "strict", but I'm not sure > this was the right decision. The documentation seems to recommend not using > "strict" (it says that weakly-linking is almost always a better API choice). libc++ is one of the exceptions

[PATCH] D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute

dexonsmith added inline comments. Comment at: test/std/language.support/support.dynamic/new.delete/new.delete.placement/new_deployment.fail.cpp:12-16 +// test availability of new/delete operators introduced in c++17. + +#ifdef __APPLE__ +#undef _LIBCPP_DISABLE_AVAILABILITY +#end

[PATCH] D34578: cmath: Support clang's -fdelayed-template-parsing

dexonsmith created this revision. r283051 added some functions to cmath (in namespace std) that have the same name as functions in math.h (in the global namespace). Clang's limited support for `-fdelayed-template-parsing` chokes on this. Rename the ones in `cmath` and their uses in `complex`

[PATCH] D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute

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

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

dexonsmith added a comment. In https://reviews.llvm.org/D41050#954863, @rjmccall wrote: > Heh, alright, that works. It's unfortunate that -disable-llvm-passes doesn't > suppress running the pass under -O0; that seems like an oversight. > > Anyway, LGTM. I suspect `-O0` just generates differen

[PATCH] D41423: [Lex] Avoid out-of-bounds dereference in LexAngledStringLiteral.

dexonsmith requested changes to this revision. dexonsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Lex/Lexer.cpp:2014-2015 +// getAndAdvanceChar. +if (C == '\\') + C = getAndAdvanceChar(CurPtr, Result); + ---

[PATCH] D41423: [Lex] Avoid out-of-bounds dereference in LexAngledStringLiteral.

dexonsmith added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:2026 + +if (C == 0) { NulCharacter = CurPtr-1; vsapsai wrote: > dexonsmith wrote: > > Should this check still be skipped (in an `else if` of the `C == '\\'` > > check)? > I agree it

[PATCH] D44263: Implement LWG 2221 - No formatted output operator for nullptr

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

[PATCH] D47157: Warning for framework headers using double quote includes

dexonsmith added inline comments. Comment at: include/clang/Basic/DiagnosticLexKinds.td:713 +def warn_quoted_include_in_framework_header : Warning< + "double-quoted include \"%0\" in framework header, expected system style include" + >, InGroup, DefaultIgnore; ---

[PATCH] D47157: Warning for framework headers using double quote includes

dexonsmith added inline comments. Comment at: include/clang/Basic/DiagnosticLexKinds.td:713 +def warn_quoted_include_in_framework_header : Warning< + "double-quoted include \"%0\" in framework header, expected system style include" + >, InGroup, DefaultIgnore; ---

[PATCH] D47157: Warning for framework headers using double quote includes

dexonsmith added inline comments. Comment at: test/Modules/double-quotes.m:24-25 + +// The same warnings show up when modules is on but -verify doesn't get it +// because they only show up under the module A building context. +// RUN: FileCheck --input-file=%t/stderr %s -

[PATCH] D47157: Warning for framework headers using double quote includes

dexonsmith added inline comments. Comment at: test/Modules/double-quotes.m:27-29 +// CHECK: double-quoted include "A0.h" in framework header, expected angle-bracketed include instead +// CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed include

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

dexonsmith added a comment. In https://reviews.llvm.org/D47687#1125926, @rnk wrote: > @dexonsmith is there someone from Apple who can comment on rdar://8678458 and > the merits of disabling this warning in macros? I strongly suspect the > original report was dealing with code like `assert(x ||

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

dexonsmith added a comment. In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote: > In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote: > > > In https://reviews.llvm.org/D47687#1125926, @rnk wrote: > > > > > @dexonsmith is there someone from Apple who can comment on rdar://8678

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

dexonsmith added a comment. In https://reviews.llvm.org/D47687#1127120, @Higuoxing wrote: > In https://reviews.llvm.org/D47687#1126607, @dexonsmith wrote: > > > In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote: > > > > > In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote: >

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

dexonsmith added reviewers: arphaman, ahatanak. dexonsmith added subscribers: arphaman, ahatanak. dexonsmith added a comment. In https://reviews.llvm.org/D47687#1133222, @Higuoxing wrote: > I think I am almost there. > > Here, In my sight > > #define foo(op0, op1, x, y, z) ( (void)(x op0 y op1

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

dexonsmith requested changes to this revision. dexonsmith added inline comments. This revision now requires changes to proceed. Comment at: lib/Sema/SemaExpr.cpp:12304-12309 // Diagnose "arg1 & arg2 | arg3" if ((Opc == BO_Or || Opc == BO_Xor) && !OpLoc.isMacroID()/*

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

dexonsmith added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12236 +/// Look for '&&' in the righ or left hand of a '||' expr +static void DiagnoseLogicalAndInLogicalOr(Sema &Self, SourceLocation OpLoc, - Please add a period at the end of the sentence. -

[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)

dexonsmith added a comment. Did you miss this comment from my previous review? > I would like to see tests like the following: > > NESTING_VOID_WRAPPER(&&, ||, i, i, i && i); // no warning. > NESTING_VOID_WRAPPER(||, &&, i, i, i || i); // no warning. Comment at: lib/Sem

[PATCH] D53048: [Basic] Split out -Wimplicit-int-conversion and -Wimplicit-float-conversion from -Wconversion

dexonsmith added inline comments. Comment at: clang/test/Sema/implicit-int-conversion.c:1-4 +// RUN: %clang_cc1 -Wconversion -Wno-implicit-int-conversion -DSMALL=char -DBIG=int -DNO_DIAG +// RUN: %clang_cc1 -Wno-conversion -Wimplicit-int-conversion -DSMALL=char -DBIG=int +// RU

[PATCH] D53354: [WIP][NOT FOR COMMIT][PROTOTYPE] clang-scan-deps: dependency scanning tool rough prototype

dexonsmith added inline comments. Comment at: lib/Lex/FilterToIncludes.cpp:628 + First = Id.Last; + auto Kind = llvm::StringSwitch(Id.Name) + .Case("include", pp_include) arphaman wrote: > arphaman wrote: > > arphaman wrote: > > > ddunbar wrote

[PATCH] D50539: [VFS] Add property 'fallthrough' that controls fallback to real file system.

dexonsmith added a comment. In https://reviews.llvm.org/D50539#1273654, @vsapsai wrote: > I was using `//` instead of `///` on purpose. Class `VFSFromYamlDirIterImpl` > resides entirely in .cpp file and isn't available outside of it. Comments are > supposed to cover implementation details and i

[PATCH] D50269: [compiler-rt][builtins] Don't #include CoreFoundation in os_version_check.c

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. This LGTM with a couple of nitpicks. Comment at: compiler-rt/lib/builtins/os_version_check.c:183 CFStringRef ProductVersion = (*CFStringCreateWithCStringNoCopyFunc

[PATCH] D56608: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO

dexonsmith added a comment. The code looks good. Can you add a test too? Might need to require “shell”. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56608/new/ https://reviews.llvm.org/D56608 ___ cfe-commits maili

[PATCH] D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation

dexonsmith added a comment. In D56802#1360316 , @rjmccall wrote: > We've tossed around the idea of doing things like this before, but I was > hoping that it wouldn't have to be specific to `NSObject` and that we'd e.g. > have an attribute that guarantees

[PATCH] D56816: [ObjC] Follow-up r350768 and allow the use of unavailable methods that are declared in a parent class from within the @implementation context

dexonsmith added a comment. In D56816#1360824 , @smeenai wrote: > My understanding is that rL349841 > accidentally started producing some spurious warnings/errors, rL350768 > fixed s

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

dexonsmith added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:990-998 + std::string Name = ("__const." + FunctionName(D.getParentFunctionOrMethod()) + + "." + D.getName()) + .str(); + llvm::GlobalVariable *InsertBefore = null

[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

dexonsmith added 1 blocking reviewer(s): arphaman. dexonsmith added a comment. In D54630#1308605 , @arphaman wrote: > Sounds convincing. > @dexonsmith What do you think? Besides maintaining correct behaviour, I think the most important thing here is th

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

dexonsmith added a comment. This patch LGTM, but before committing I suggest sending an RFC to cfe-dev and waiting a few days. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55455/new/ https://reviews.llvm.org/D55455 ___

[PATCH] D54604: Automatic variable initialization

dexonsmith added inline comments. Comment at: include/clang/Basic/DiagnosticDriverKinds.td:412 + "Enable it at your own peril for benchmarking purpose only with " + "-enable-trivial-auto-var-init-zero-knowning-it-will-be-removed-from-clang">; + s/knowning/knowi

[PATCH] D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache

dexonsmith created this revision. dexonsmith added reviewers: bruno, rsmith, vsapsai, Bigcheese. Herald added subscribers: jdoerfert, jsji, kbarton, mgorny, nemanjai. Change MemoryBufferCache to InMemoryModuleCache, moving it from Basic to Serialization. Another patch will start using it to manag

[PATCH] D58891: Modules: Add -Rmodule-import

dexonsmith created this revision. dexonsmith added reviewers: Bigcheese, bruno, rsmith, vsapsai. Herald added a subscriber: jdoerfert. Add a remark for importing modules. Depending on whether this is a direct import (into the TU being built by this compiler instance) or transitive import (into an

[PATCH] D58893: Modules: Invalidate out-of-date PCMs as they're discovered

dexonsmith created this revision. dexonsmith added reviewers: Bigcheese, bruno, rsmith, vsapsai. Herald added a subscriber: jdoerfert. dexonsmith added parent revisions: D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache, D58891: Modules: Add -Rmodule-import. Leverage the InMemoryMo

[PATCH] D58891: Modules: Add -Rmodule-import

dexonsmith marked an inline comment as done. dexonsmith added inline comments. Comment at: clang/test/Modules/Rmodule-build.m:22-25 -// RUN: echo ' ' >> %t/C.h -// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fsyntax-only %s -I %t \ -// RUN:

[PATCH] D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache

dexonsmith added a comment. In D58890#1417465 , @bruno wrote: > `InMemoryModuleCache` seems like a way more appropriate name here. Also > thanks for improving some of the comments. > > > Because of the move to Serialization we can no longer abuse the > >

[PATCH] D58891: Modules: Add -Rmodule-import

dexonsmith closed this revision. dexonsmith marked 2 inline comments as done. dexonsmith added a comment. Committed in r355477. Comment at: clang/test/Modules/Inputs/Rmodule-import/B.h:1 +// B +#include "C.h" bruno wrote: > Can you make one of the tests to use

[PATCH] D59032: Passthrough compiler launcher

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59032/new/ https://reviews.llvm.org/D59032

[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

dexonsmith created this revision. dexonsmith added reviewers: bruno, rsmith, vsapsai, jordan_rose. Herald added a subscriber: jdoerfert. Add an option to cache the generated PCH in the ModuleCache when emitting it. This protects clients that build PCHs and read them in the same process, allowing

[PATCH] D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache

dexonsmith closed this revision. dexonsmith added a comment. Committed in r355777. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58890/new/ https://reviews.llvm.org/D58890 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D58893: Modules: Invalidate out-of-date PCMs as they're discovered

dexonsmith closed this revision. dexonsmith marked 2 inline comments as done. dexonsmith added a comment. Committed in r355778. Comment at: clang/include/clang/Serialization/InMemoryModuleCache.h:37 /// Track the timeline of when this was added to the cache. +bool Is

[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

dexonsmith marked 2 inline comments as done. dexonsmith added a comment. In D59176#1424864 , @jordan_rose wrote: > This commit by itself doesn't change any behavior, right? Correct, it just exposes an option. Comment at: clang/lib/Fro

[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

dexonsmith updated this revision to Diff 190203. dexonsmith added a comment. Updated the constructor call to `PCHGenerator` in `GenerateModuleAction::CreateASTConsumer` to use `BuildingImplicitModule` on its own. Checking where it's set (only in `compileModuleImpl`), it's exactly the condition

[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

dexonsmith marked 2 inline comments as done. dexonsmith added inline comments. Comment at: clang/lib/Frontend/FrontendActions.cpp:182 + CI.getFrontendOpts().BuildingImplicitModule && + CI.getLangOpts().isCompilingModule())); Consumers.push_back(CI.getPCHContainer

[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

dexonsmith closed this revision. dexonsmith added a comment. Committed in r355950. Thanks for the review. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59176/new/ https://reviews.llvm.org/D59176 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

dexonsmith added a comment. In D58514#1428434 , @ahatanak wrote: > Seems like the chromium code is valid and shouldn't crash. John/Erik what do > you think? The following code also crashes with this patch applied. > > typedef void (^BlockTy)(); > >

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

dexonsmith added a comment. In D58514#1428520 , @ahatanak wrote: > That code doesn't crash because the block is retained at the entry of `bar` > and ARC optimizer doesn't remove the retain/release pairs in `bar`. Oops, I meant: typedef void (^BlockTy

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

dexonsmith added a comment. In D58514#1428567 , @ahatanak wrote: > Sorry, I misread the code. If you change the parameter type of `bar` to > `BlockTy`, the code crashes. If the type is `id`, it doesn't crash because > IRGen copies the block to the heap i

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

dexonsmith created this revision. Herald added subscribers: jdoerfert, kadircet, arphaman, jkorous. Remove CompilerInstance::VirtualFileSystem and CompilerInstance::setVirtualFileSystem, instead relying on the VFS in the FileManager. CompilerInstance and its clients already went to some trouble t

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

dexonsmith updated this revision to Diff 190726. dexonsmith added a comment. I slightly simplified the code for CompilerInstance::createFileManager (replaced with an `assert`), since I noticed that FileManager always constructs its own VFS if it isn't passed one. CHANGES SINCE LAST ACTION ht

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

dexonsmith created this revision. dexonsmith added reviewers: arphaman, Bigcheese, bruno. Herald added subscribers: jdoerfert, kadircet, jkorous, ioeric. Herald added a reviewer: martong. Herald added a reviewer: shafik. FileManager constructs a VFS in its constructor if it isn't passed one, and t

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

dexonsmith added a comment. In D59388#1431314 , @jkorous wrote: > Hi Duncan, thanks for working on better interfaces in clang! > > I am just wondering - is it safe to have the lifetime of a single object on > heap managed by two different `IntrusiveRefCnt

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

dexonsmith updated this revision to Diff 191104. dexonsmith added a comment. Document the default VFS used by the `FileManager`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59377/new/ https://reviews.llvm.org/D59377 Files: clang-tools-extra/clangd/Compiler.cpp clang/include/clang

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

dexonsmith added a comment. In D59377#1431203 , @jkorous wrote: > In D59377#1430002 , @dexonsmith > wrote: > > > ... since I noticed that FileManager ... > > > This kind of implies that we should move the comment f

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

dexonsmith updated this revision to Diff 191105. dexonsmith added a comment. Rebase. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59388/new/ https://reviews.llvm.org/D59388 Files: clang-tools-extra/clang-move/ClangMove.cpp clang-tools-extra/clang-tidy/ClangTidy.cpp clang-tools-e

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

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

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

dexonsmith edited reviewers, added: erik.pilkington, arphaman; removed: dexonsmith. dexonsmith added a comment. +Erik and Alex, can you take a look at this? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59628/new/ https://reviews.llvm.org/D59628 _

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

dexonsmith closed this revision. dexonsmith added a comment. Committed r357037. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59377/new/ https://reviews.llvm.org/D59377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

dexonsmith closed this revision. dexonsmith added a comment. Committed r357038. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59388/new/ https://reviews.llvm.org/D59388 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

dexonsmith added a comment. In D55463#1451166 , @tschuett wrote: > Are you planning to do this recursively? > The minimizer does not help much for the following example, while Sema.h > contains 10,000 lines of useless code. > > #include "clang/Sema/Sem

[PATCH] D60233: [clang-scan-deps] initial outline of the tool that runs preprocessor to find dependencies over a JSON compilation database

dexonsmith added a comment. In D60233#1467092 , @trixirt wrote: > A comment by whisperity in the origin WIP did not seem to be addressed. > Have you looked at IWYU > https://github.com/include-what-you-use/include-what-you-use ? > The end goals of clang

[PATCH] D60748: Fix i386 struct and union parameter alignment

dexonsmith added a comment. In D60748#1472829 , @rjmccall wrote: > I suspect Darwin also doesn't want to take this. We care very little about > 32-bit Intel, and part of caring very little is not wanting to spend any > effort dealing with the ramificati

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

dexonsmith added a comment. In D61165#1492724 , @rjmccall wrote: > In D61165#1492582 , @erik.pilkington > wrote: > > > Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which > > appears to claim t

  1   2   3   4   5   6   7   8   9   10   >