[PATCH] D49240: [libc++] Introduce _LIBCPP_HIDE_FROM_ABI to replace _LIBCPP_INLINE_VISIBILITY

thakis added a comment. When we updated out clang bundle in chromium (which includes libc++ headers), our ios simulator bots regressed debug info size by ~50% due to this commit (https://bugs.chromium.org/p/chromium/issues/detail?id=872926#c13). Is that expected? Repository: rCXX libc++ ht

[PATCH] D46652: [clang-cl, PCH] Implement support for MS-style PCH through headers

thakis added a comment. In https://reviews.llvm.org/D46652#1174220, @mikerice wrote: > In https://reviews.llvm.org/D46652#1164010, @thakis wrote: > > > Also, were you planning on also adding support for the (filename-less > > version of) hdrstop pragma? After this change, that should probably be

[PATCH] D50640: Fix for bug 38508 - Don't do PCH processing when only generating preprocessor output

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. The approach lgtm, thanks. How does the gcc driver codepath handle this? Does it just not have to worry about this because it doesn't have something like warn_pp_macro_def_mismatch_with_pch?

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

thakis added a comment. I haven't read all the messages in these threads, forgive me if someone asked this already. It's a bit weird to me that we have to override this behavior in Chromium while the default is different. Why isn't the executable size blowup we see in chromium a problem for eve

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

thakis added a comment. (I defer to Hans) Repository: rCXX libc++ https://reviews.llvm.org/D50652 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50640: Fix for bug 38508 - Don't do PCH processing when only generating preprocessor output

thakis accepted this revision. thakis added a comment. Thanks! Do you need someone to land this? https://reviews.llvm.org/D50640 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50907: Make __shiftleft128 / __shiftright128 real compiler built-ins.

thakis added a comment. Forgot to add cfe-commits :-( Doing that now. https://reviews.llvm.org/D50907 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50907: Make __shiftleft128 / __shiftright128 real compiler built-ins.

thakis added a comment. r340048, thanks! https://reviews.llvm.org/D50907 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50877: [MS] Mangle a hash of the main file path into anonymous namespaces

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Can you explicitly mention that this intentionally doesn't use an absolute path in MicrosoftMangleContextImpl() or similar? https://reviews.llvm.org/D50877

[PATCH] D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

thakis created this revision. thakis added a reviewer: rnk. Herald added a reviewer: javed.absar. Herald added a subscriber: kristof.beyls. EmitX86BuiltinExpr() emits all args into Ops at the beginning, so don't do that work again. No intended behavior change. (TNorthover: EmitAArch64BuiltinExpr

[PATCH] D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

thakis added a comment. In https://reviews.llvm.org/D50979#1206211, @rsmith wrote: > I don't think this is NFC. Testcase: > > long long int a, b, c, d; > unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, > &(++d)); } > > > Today, Clang increments `a`, `b`, `c`, and

[PATCH] D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

thakis added a comment. In https://reviews.llvm.org/D50979#1206282, @thakis wrote: > In https://reviews.llvm.org/D50979#1206211, @rsmith wrote: > > > I don't think this is NFC. Testcase: > > > > long long int a, b, c, d; > > unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++

[PATCH] D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

thakis updated this revision to Diff 161824. thakis edited the summary of this revision. thakis added a comment. Add tests. https://reviews.llvm.org/D50979 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/test/CodeGen/ms-intrinsics.c clang/test/CodeGen/ms-x86-intrinsics.c Index: clang/test/C

[PATCH] D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

thakis closed this revision. thakis added a comment. r340348, thanks! https://reviews.llvm.org/D50979 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51134: win: Omit ".exe" from clang and clang-cl driver-level diagnostics.

thakis created this revision. thakis added a reviewer: hans. Like https://reviews.llvm.org/D51133 but for clang. https://reviews.llvm.org/D51134 Files: clang/tools/driver/driver.cpp Index: clang/tools/driver/driver.cpp === ---

[PATCH] D51134: win: Omit ".exe" from clang and clang-cl driver-level diagnostics.

thakis closed this revision. thakis added a comment. r340498, thanks! https://reviews.llvm.org/D51134 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46050: [Frontend] Avoid running plugins during code completion parse

thakis accepted this revision. thakis added inline comments. This revision is now accepted and ready to land. Comment at: test/Index/complete-and-plugins.c:3 + +// RUN: c-index-test -code-completion-at=%s:6:1 -load %llvmshlibdir/PrintFunctionNames%pluginext -add-plugin print-fns

[PATCH] D46820: Fix __uuidof handling on non-type template parameter in C++17

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. lgtm, but: Comment at: test/SemaCXX/PR24986.cpp:12 + f<&__uuidof(0)>(); +} Maybe this test could be in test/SemaCXX/ms-uuid.cpp instead? Repository: rC

[PATCH] D46820: Fix __uuidof handling on non-type template parameter in C++17

thakis closed this revision. thakis added a comment. r332614, thanks! Repository: rC Clang https://reviews.llvm.org/D46820 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46929: Fix a mangling failure on clang-cl C++17

thakis added a comment. Please do, tzik doesn't have commit access yet. Repository: rC Clang https://reviews.llvm.org/D46929 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46910: [Support] Avoid normalization in sys::getDefaultTargetTriple

thakis added a comment. This changes the default triple on Windows from x86_64-pc-win32 to x86_64-pc-windows-msvc. This breaks at least Repository: rC Clang https://reviews.llvm.org/D46910 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46910: [Support] Avoid normalization in sys::getDefaultTargetTriple

thakis added a comment. (sorry, hit cmd-return accidentally, I will come in again…) Repository: rC Clang https://reviews.llvm.org/D46910 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co

[PATCH] D46910: [Support] Avoid normalization in sys::getDefaultTargetTriple

thakis added a comment. This changes the default triple on Windows from x86_64-pc-win32 to x86_64-pc-windows-msvc. This breaks at least lit's make_itanium_abi_triple() (http://llvm-cs.pcc.me.uk/utils/lit/lit/llvm/config.py#234) def make_itanium_abi_triple(self, triple): m = re.match(r'(

[PATCH] D46910: [Support] Avoid normalization in sys::getDefaultTargetTriple

thakis added a comment. …hm looks like https://reviews.llvm.org/rL332750 touched files in both clang and llvm, which is probably why the diff looks super confusing on phab. I'll revert it in two pieces, I don't know how to commit changes to llvm and clang in one revision. Repository: rC Cla

[PATCH] D46910: [Support] Avoid normalization in sys::getDefaultTargetTriple

thakis added a comment. Reverted in r332822 / r332823. Repository: rC Clang https://reviews.llvm.org/D46910 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46910: [Support] Avoid normalization in sys::getDefaultTargetTriple

thakis added a comment. > Sorry about the breakage, I wasn't aware of that test failure and never got > any email (probably because the bot has been red before). I'll try to fix > make_itanium_abi_triple and test it on my Windows box tomorrow before > resubmitting. Thanks, sounds good. Reading

[PATCH] D47480: clang-cl: Expose -no-canonical-prefixes

thakis created this revision. thakis added a reviewer: rnk. -no-canonical-prefixes is a weird flag: In gcc, it controls whether realpath() is called on the path of the driver binary. It's needed to support some usecases where gcc is symlinked to, see https://gcc.gnu.org/ml/gcc/2011-01/msg00429.

[PATCH] D47480: clang-cl: Expose -no-canonical-prefixes

thakis added a comment. I did test this locally before sending it out, and the -resource-dir arg passed to cc1 is relative with this patch: $ bin/clang-cl -no-canonical-prefixes -c test.cc -### clang version 7.0.0 Target: x86_64-pc-windows-msvc Thread model: posix InstalledDir: bin

[PATCH] D47480: clang-cl: Expose -no-canonical-prefixes

thakis added a subscriber: rnk. thakis added a comment. But GetExecutablePath isn't called if -no-canonical-prefixes is passed, is it? (See the first link I pasted above) https://reviews.llvm.org/D47480 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D38291: clang-format/java: Unbreak genenrics formatting after r299952.

thakis created this revision. Herald added a subscriber: klimek. https://reviews.llvm.org/rL299952 merged '>>>' tokens into a single JavaRightLogicalShift token. This broke formatting of generics nested more than two deep, e.g. `Foo>>` because the '>>>' now weren't three '>' for parseAngle().

[PATCH] D38291: clang-format/java: Unbreak genenrics formatting after r299952.

thakis closed this revision. thakis added a comment. r314325, thanks! https://reviews.llvm.org/D38291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38646: [MS] Raise the default value of _MSC_VER to 1910, which is in VS 2017

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Thanks! Should probably get a release note too. https://reviews.llvm.org/D38646 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D38954: [Sema] -Wzero-as-null-pointer-constant: don't warn for system macros.

thakis added a comment. As said on the bug, this matches gcc's behavior, and with this you won't see this warning for NULL. I don't think this is better. Repository: rL LLVM https://reviews.llvm.org/D38954 ___ cfe-commits mailing list cfe-commit

[PATCH] D18953: [ms][dll] #26935 Defining a dllimport function should cause it to be exported

thakis added inline comments. Comment at: cfe/trunk/lib/Sema/SemaDecl.cpp:5650 + NewImportAttr->getRange(), S.Context, + NewImportAttr->getSpellingListIndex())); +} else { NewImportAttr can be nullptr here, at least for invalid code. This ch

[PATCH] D39104: Allow /showIncludes with /P

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Thanks! https://reviews.llvm.org/D39104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

[PATCH] D51545: Enable -Wtautological-unsigned-zero-compare under -Wextra

thakis added a comment. We don't match gcc's -Wextra behvior. We generally try to not put a ton of stuff in Wextra that isn't in -Wall. (We also generally don't put a lot of stuff in -Wall that isn't enabled by default.) So I don't think we want this. https://reviews.llvm.org/D51545 ___

[PATCH] D51635: clang-cl: Pass /Brepro to linker if it was passed to the compiler

thakis created this revision. thakis added a reviewer: hans. /Brepro currently is just an alias for -mno-incremental-linker-compatible and before this patch only controlled if we write a timestamp into the output obj file. But cl.exe also passes it on to link.exe (where it controls whether the

[PATCH] D51354: Fix the -print-multi-directory flag to print the selected multilib.

thakis added a comment. The test fails on my system like so: -- FAIL: Clang :: Driver/print-multi-directory.c (4696 of 13174) TEST 'Clang :: Driver/print-multi-directory.c' FAILED Script: -- : 'RUN: at line 1'; /u

[PATCH] D45179: [libc++] Add _LIBCPP_ENABLE_NODISCARD and _LIBCPP_NODISCARD_EXT to allow pre-C++2a [[nodiscard]]

thakis added a comment. What's the status here? Did this land? https://reviews.llvm.org/D45179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51784: ms: Insert $$Z in mangling between directly consecutive parameter packs.

thakis created this revision. thakis added a reviewer: rnk. Fixes PR38783. For comparing: https://godbolt.org/z/XBSdiq https://reviews.llvm.org/D51784 Files: clang/lib/AST/MicrosoftMangle.cpp clang/test/CodeGenCXX/mangle-ms-templates.cpp Index: clang/test/CodeGenCXX/mangle-ms-templates.c

[PATCH] D51784: ms: Insert $$Z in mangling between directly consecutive parameter packs.

thakis added inline comments. Comment at: clang/test/CodeGenCXX/mangle-ms-templates.cpp:201 + multi_variadic_mixed(1, 2, 3); + multi_variadic_mixed(1, 2, 3, 4); } These notably don't get a $$Z because there's a regular template parameter in between. https:/

[PATCH] D49597: [ms] Fix mangling of vector types in QMM_Result contexts.

thakis created this revision. thakis added a reviewer: zturner. If QMM_Result is set (which it is for return types, RTTI descriptors, and exception type descriptors), tag types (structs, enums, classes, unions) get their qualifiers mangled in. __m64 and friends is a struct/union thingy in MSVC,

[PATCH] D49597: [ms] Fix mangling of vector types in QMM_Result contexts.

thakis added a comment. If you want to compare the CHECK lines to MSVC's output: https://godbolt.org/g/Cvf4p4 https://reviews.llvm.org/D49597 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

[PATCH] D49606: [ms] Add __shiftleft128 / __shiftright128 intrinsics

thakis created this revision. thakis added a reviewer: craig.topper. cl.exe maps these to shld / shrd, so let's do the same. ISel has `Subtarget->isSHLDSlow()` to prevent use of these intrinsics on some machines, but honoring them feels a bit like trying to outsmart the intrinsics user, and the

[PATCH] D49606: [ms] Add __shiftleft128 / __shiftright128 intrinsics

thakis added a comment. We have __int128. If you think hitting the pattern is preferable to inline asm, I can try to give that a try, either via C or via CGBuiltin.cpp. https://reviews.llvm.org/D49606 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D49606: [ms] Add __shiftleft128 / __shiftright128 intrinsics

thakis updated this revision to Diff 156596. thakis edited the summary of this revision. https://reviews.llvm.org/D49606 Files: clang/lib/Headers/intrin.h clang/test/Headers/ms-intrin.cpp Index: clang/test/Headers/ms-intrin.cpp ===

[PATCH] D49606: [ms] Add __shiftleft128 / __shiftright128 intrinsics

thakis added a comment. Now with C builtins which get nicely optimized. https://reviews.llvm.org/D49606 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49606: [ms] Add __shiftleft128 / __shiftright128 intrinsics

thakis added a comment. Isn't implementing this in plain old C the nicest approach anyhow, even once funnel shift exists? https://reviews.llvm.org/D49606 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D49606: [ms] Add __shiftleft128 / __shiftright128 intrinsics

thakis closed this revision. thakis added a comment. r337619, thanks! The hoisting point is a good one; will rewrite using funnelshift once that's possible :-) https://reviews.llvm.org/D49606 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49597: [ms] Fix mangling of vector types in QMM_Result contexts.

thakis added a comment. rnk, since zturner is out until Thu, can you take a look? https://reviews.llvm.org/D49597 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49597: [ms] Fix mangling of vector types in QMM_Result contexts.

thakis updated this revision to Diff 156858. thakis added a comment. Address comments. https://reviews.llvm.org/D49597 Files: clang/lib/AST/MicrosoftMangle.cpp clang/test/CodeGenCXX/mangle-ms-vector-types.cpp Index: clang/test/CodeGenCXX/mangle-ms-vector-types.cpp =

[PATCH] D49597: [ms] Fix mangling of vector types in QMM_Result contexts.

thakis marked 2 inline comments as done. thakis added a comment. Thanks! All done. https://reviews.llvm.org/D49597 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49597: [ms] Fix mangling of vector types in QMM_Result contexts.

thakis closed this revision. thakis added a comment. r337732, thanks! https://reviews.llvm.org/D49597 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41102: Setup clang-doc frontend framework

thakis added a comment. This should be in clang-tools-extra next to clang-tidy, clang-include-fixer, clangd etc, not in the main compiler repo, right? https://reviews.llvm.org/D41102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

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

thakis added a comment. I went ahead and landed this with the comments requested by Duncan in r324385. (http://llvm.org/viewvc/llvm-project?view=revision&revision=324385) I'm also quoting Duncan's email reply to my comment, since that's not showing up on phab: """ Using runtime availability che

[PATCH] D36191: [CodeGen] Don't make availability attributes imply default visibility on macos

thakis added a comment. Herald added a subscriber: llvm-commits. What's the status here? Repository: rL LLVM https://reviews.llvm.org/D36191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

[PATCH] D36191: [CodeGen] Don't make availability attributes imply default visibility on macos

thakis added a comment. Err sorry, landed in https://reviews.llvm.org/rL310382. Repository: rL LLVM https://reviews.llvm.org/D36191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43110: [Sema] Don't mark plain MS enums as fixed

thakis added a comment. Nice! Test? https://reviews.llvm.org/D43110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43221: Teach Wreturn-type, Wunreachable-code, and alpha.deadcode.UnreachableCode to treat __assume(0) like __builtin_unreachable.

thakis created this revision. thakis added reviewers: rsmith, rnk. Herald added a subscriber: whisperity. Fixes PR29134. https://reviews.llvm.org/D43221 Files: include/clang/AST/Expr.h lib/AST/Expr.cpp lib/Analysis/CFG.cpp lib/Analysis/ReachableCode.cpp lib/StaticAnalyzer/Checkers/Unr

[PATCH] D43221: Teach Wreturn-type, Wunreachable-code, and alpha.deadcode.UnreachableCode to treat __assume(0) like __builtin_unreachable.

thakis closed this revision. thakis added a comment. r325052, thanks! https://reviews.llvm.org/D43221 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

thakis added a comment. FWIW we build with -Wall -Wextra and we disable this warning since it's in our (chromium's) opinion not useful on large real-world code. So I'm not sure it should be in -Wextra. (Also, I believe clang has historically tried to keep -Wall and -Wextra pretty similar?) Re

[PATCH] D43888: [clang-cl] Implement /X

thakis created this revision. thakis added a reviewer: hans. /X makes cl stop looking in %INCLUDE%. Implement this for clang-cl. As it turns out, the return in ToolChains/MSVC.cpp, AddClangSystemIncludeArgs() for -nostdlibinc is already in the right place (but -nostdlibinc isn't exposed by clan

[PATCH] D43888: [clang-cl] Implement /X

thakis closed this revision. thakis added a comment. r326357, thanks! https://reviews.llvm.org/D43888 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43980: Push a function scope when parsing function bodies without a declaration

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Fix LGTM, one optional comment below. Comment at: clang/lib/Sema/SemaDecl.cpp:12412 +// anyway so we can try to parse the function body. +PushFunctionScope(); re

[PATCH] D44039: [Sema] Make getCurFunction() return null outside function parsing

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Good luck! I think the forward-looking statement in https://reviews.llvm.org/D43980 about this change here is a bit off, see below. The hasLocalStorage() comment below is probably the only in

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

thakis created this revision. thakis added a reviewer: rnk. No effective behavior change, just for cleanliness. Fixes PR36159. https://reviews.llvm.org/D44223 Files: lib/CodeGen/MicrosoftCXXABI.cpp Index: lib/CodeGen/MicrosoftCXXABI.cpp =

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

thakis added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1207 + + for (const VBaseEntry E : VBases) { +if (!E.second.hasVtorDisp()) (I added the missing `&` here locally.) https://reviews.llvm.org/D44223

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

thakis updated this revision to Diff 137463. thakis added a comment. Herald added a subscriber: mgrang. sort unstably https://reviews.llvm.org/D44223 Files: lib/CodeGen/MicrosoftCXXABI.cpp Index: lib/CodeGen/MicrosoftCXXABI.cpp ===

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

thakis updated this revision to Diff 137470. thakis added a comment. rnk comment https://reviews.llvm.org/D44223 Files: lib/CodeGen/MicrosoftCXXABI.cpp Index: lib/CodeGen/MicrosoftCXXABI.cpp === --- lib/CodeGen/MicrosoftCXXABI.

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

thakis marked an inline comment as done. thakis added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1206 + + for (const VBase &V : VBases) { +if (!V.second.hasVtorDisp()) rnk wrote: > I think we can avoid the sort altogether if we iterate `RD-

[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

thakis closed this revision. thakis marked an inline comment as done. thakis added a comment. r326960, thanks! https://reviews.llvm.org/D44223 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

[PATCH] D16403: Add scope information to CFG

thakis added a comment. Since this affects analysis-based warnings, have you checked if this patch has any effect on compile times? Repository: rL LLVM https://reviews.llvm.org/D16403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D44630: [ms] Parse #pragma optimize and ignore it behind its own flag

thakis accepted this revision. thakis added a comment. Awesome, thanks! One nit below: Comment at: include/clang/Basic/DiagnosticParseKinds.td:973 +def warn_pragma_optimize : Warning< + "'#pragma optimize' is not supported; use '#pragma clang optimize on|off' instead">, + In

[PATCH] D44826: Add -Wunused-using, a warning that finds unused using declarations.

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Looks great. Comment at: docs/ReleaseNotes.rst:126 +namespace n { class C; } +using n::C; // Never actually used. + Maybe include an example with a

[PATCH] D47480: clang-cl: Expose -no-canonical-prefixes

thakis closed this revision. thakis added a comment. r333761, thanks! https://reviews.llvm.org/D47480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47480: clang-cl: Expose -no-canonical-prefixes

thakis marked 2 inline comments as done. thakis added inline comments. Comment at: test/Driver/cl-options.c:595 +// RUN: -no-canonical-prefixes \ +// RUN: -fno-coverage-mapping \ // RUN: --version \ rnk wrote: > takuto.ikuta wrote: > > Is this relate

[PATCH] D47956: [MS] Consder constexpr globals to be inline, as in C++17

thakis added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6597 + (getLangOpts().CPlusPlus17 || + Context.getTargetInfo().getCXXABI().isMicrosoft())) NewVD->setImplicitlyInline(); Is this related to /Zc:externConstexpr / PR3

[PATCH] D47578: Do not enforce absolute path argv0 in windows

thakis added a comment. ruiu: This review has now gone on for a week, with one cycle per day due to timezones. Since the comments are fairly minor nits, do you think you could address them yourself while landing this, in the interest of not spending another week on this? https://reviews.llvm.

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

thakis added a comment. What's the status here? The MS STL has this feature under _HAS_NODISCARD and it's super useful. Repository: rCXX libc++ https://reviews.llvm.org/D45179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D48224: Don't let test/Driver/no-canonical-prefixes.c form a symlink cycle the second time it runs.

thakis created this revision. thakis added a reviewer: chandlerc. The test makes %t.fake a symlink to %t.real by running `ln -sf %t.real %t.fake`. If %t.fake already is a symlink to %t.real when this runs (e.g. if the test has run before), then this effectively becomes `ln -sf %t.real %t.real`,

[PATCH] D48224: Don't let test/Driver/no-canonical-prefixes.c form a symlink cycle the second time it runs.

thakis closed this revision. thakis added a comment. r334972, thanks! https://reviews.llvm.org/D48224 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48296: clang-cl: Emit normal narrowing diagnostics for initializer lists if -fmsc-version is at least 1900 (i.e. MSVC2015).

thakis created this revision. thakis added a reviewer: rnk. Diagnostics for narrowing conversions in initializer lists are currently DefaultIgnored in Microsoft mode. But MSVC 2015 did add warnings about narrowing conversions (C2397), so clang-cl can remove its special case code if MSCompatibil

[PATCH] D48296: clang-cl: Emit normal narrowing diagnostics for initializer lists if -fmsc-version is at least 1900 (i.e. MSVC2015).

thakis added a comment. Thanks! I'd keep it DefaultIgnored: I'd be annoyed if I'd get these by default in C++98 mode. https://reviews.llvm.org/D48296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

[PATCH] D48296: clang-cl: Emit normal narrowing diagnostics for initializer lists if -fmsc-version is at least 1900 (i.e. MSVC2015).

thakis updated this revision to Diff 151844. thakis marked 2 inline comments as done. thakis added a comment. comments https://reviews.llvm.org/D48296 Files: clang/lib/Sema/SemaInit.cpp clang/test/SemaCXX/ms-initlist-narrowing.cpp Index: clang/test/SemaCXX/ms-initlist-narrowing.cpp ==

[PATCH] D48296: clang-cl: Emit normal narrowing diagnostics for initializer lists if -fmsc-version is at least 1900 (i.e. MSVC2015).

thakis closed this revision. thakis added a comment. r335082, thanks! https://reviews.llvm.org/D48296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47956: [MS] Consder constexpr globals to be inline, as in C++17

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Thanks for explaining, makes sense to me. https://reviews.llvm.org/D47956 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

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

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Looks like pretty straightforward plumbing. I can't think of a better place than ASTContext either. I think this is fine with your two XXXs just removed. Exciting build time wins! =

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

thakis added a comment. PCHs aren't compatible with themselves if only the compiler revision changes, so I'm not sure changing that field should be worse than a regular compiler revision update (which happens at every commit). But I don't know what this field is for. I don't remember any troubl

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

thakis added inline comments. Comment at: lib/AST/ASTContext.cpp:9563 +if (getExternalSource()->DeclIsFromPCHWithObjectFile(D)) + return false; + } It'd be good to add a comment (and a test) explaining why / how _referenced_ inline functions still get

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

thakis added inline comments. Comment at: test/CodeGen/pch-dllexport.cpp:41 + +void use() { baz(); } + There still isn't a non-dllexported-but-referenced inline function in here as far as I can see. Am I just missing it? https://reviews.llvm.org/D48426

[PATCH] D47698: [ASTImporter] import macro source locations

thakis added a comment. This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h? Repository: rC Clang https://reviews.llvm.org/D47698 ___ cfe-commits mailing li

[PATCH] D53434: Java annotation declaration being handled correctly

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Thanks for the fix! Do you happen to know what had regressed this? Repository: rC Clang https://reviews.llvm.org/D53434 ___ cfe-commits mailin

[PATCH] D53457: clang-cl: Add "/Xdriver:" pass-through arg support.

thakis added a comment. Comment from the peanut gallery: - I like the whitelist model for gcc-style flags. It allows us to curate them (and `/?` output) since many don't make much sense in the cl world. - I like the idea behind this patch (and /clang: seems like a good spelling) Repository:

[PATCH] D55491: Implement TemplateArgument dumping in terms of Visitor

thakis added a comment. Out of interest, what's the motivation for this? It seems to add way more code than it removes, so there must be some other advantage, but the patch description doesn't say. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55491/new/ https:

[PATCH] D56732: [clang] [test] Disable Python binding tests w/ LLVM_ENABLE_PIC=OFF

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Thanks! I can't easily test this since only some of our bots see this and I don't have a Linux box at hand, but it looks like it should work and I can verify on the bots once this is in :)

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Thanks for the fix! Nice test too. Let's land this and see if anything breaks :-) On the 8.0, I'd probably recommend reverting your change instead, since this feels like a subtle change that

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

thakis added inline comments. Comment at: lib/Basic/FileManager.cpp:295 + llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true); + UFE.RealPathName = AbsPath.str(); +} Should this call fillRealPathName() (which was added after your change)?

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. lgtm, but lebtm with call to fillRealPathName() Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57165/new/ https://reviews.llvm.org/D57165 __

[PATCH] D57127: [analyzer] Port RetainSummaryManager to the new GenericCall interface, decouple ARCMT from the analyzer

thakis added a comment. Can you elaborate a bit in what sense this decouples ARCMT from the analyzer? Can CLANG_ENABLE_STATIC_ANALYZER now be set independently of CLANG_ENABLE_ARCMT? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57127/new/ https://reviews.llvm.o

[PATCH] D57127: [analyzer] Port RetainSummaryManager to the new GenericCall interface, decouple ARCMT from the analyzer

thakis added a comment. Also, isn't lib/Analysis a strange place for this? lib/Analysis is used by the compiler proper (CodeGen, Sema, …), while RetainSummaryManager is only used by tooling-like things (static analyzer, arcmigrate). Repository: rC Clang CHANGES SINCE LAST ACTION https://r

  1   2   3   4   5   6   7   8   9   10   >