[PATCH] D132399: [clang/test] Correctly specify simulator env in target flag in fsanitize.c

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132399/new/ https://reviews.llvm.org/D132399 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D132400: [clang] Remove a FIXME that we can't fix

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm "No behavior change." occurs twice in the change description. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132400/new/ https://reviews.llvm.org/D132400 ___

[PATCH] D132754: [clang] Add __is_target_variant_{os,environment} builtins

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132754/new/ https://reviews.llvm.org/D132754 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D134394: Update docs about [[likely]] vs. PGO

hans created this revision. hans added reviewers: Mordante, aaron.ballman. Herald added a subscriber: wenlei. Herald added a project: All. hans requested review of this revision. Herald added a project: clang. It sounds like these attributes do nothing when PGO is enabled, which is actually (or p

[PATCH] D133959: Add clang flag equivalent to clang-cl /Zl flag

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/include/clang/Driver/Options.td:2226 +def fms_omit_default_lib : Joined<["-"], "fms-omit-default-lib">, + Group, Flags<[NoXarchOption, CoreOption]>; defm

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

hans added reviewers: Mordante, aaron.ballman. hans added a comment. Looks reasonable to me. +the folks from D85091 fyi or if they want to take a look. Comment at: clang/test/Profile/cxx-never-executed-branch.cpp:1 +// Test the clang doesn't

[PATCH] D134394: Update docs about [[likely]] vs. PGO

hans added a comment. See also D134456 which affects the behavior here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134394/new/ https://reviews.llvm.org/D134394 ___

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

hans added a comment. In D131465#3808381 , @aaron.ballman wrote: > In D131465#3806709 , @MaskRay wrote: > >> In D131465#3804893 , @glandium >> wrote: >> >>> This didn't

[PATCH] D134468: [Driver] Ignore -fmsc-version= -fms-compatibility-version= values smaller than 19

hans added a comment. I'm conflicted about this. I see where you're coming from, but I could imagine people using clang-cl with old VS versions e.g. to target old Windows. I agree we shouldn't go out of our way to support that, but do we think it's too much effort to keep the functionality we h

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

hans added a comment. > If we're all using `clang` for the command, we'd all get the same behavior, > right? There's no expectation that `clang` and `cl` behave the same way, > that's what `clang-cl` is for. I guess I don't see why we care about how MSVC > behaves unless passing `-fms-compatibi

[PATCH] D134544: [clang-cl] Implement /ZH: flag

hans added a comment. nice! Comment at: clang/include/clang/Basic/CodeGenOptions.def:344 +/// Set debug info source file hashing algorithm +ENUM_CODEGENOPT(DebugSrcHashAlgorithm, SrcHashAlgorithm, 2, CSK_MD5) ultra nit: period. But maybe the comment is superf

[PATCH] D134544: [clang-cl] Implement /ZH: flag

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/include/clang/Basic/CodeGenOptions.h:106 + enum SrcHashAlgorithm { +CSK_MD5, +CSK_SHA1, thakis wrote: > hans wrote: > > what does

[PATCH] D133711: [Sema] Reject array element types whose sizes aren't a multiple of their alignments

hans added a comment. This also fires in non-array cases: $ cat /tmp/a.cc struct T0 { char c; }; struct T2 : virtual T0 { }; T2* f() { return new T2(); } $ /work/llvm-project/build/bin/clang++ -c -target i686-pc-win32 /tmp/a.cc /tmp/a.cc:4:22: error: size of array element of typ

[PATCH] D134468: [Driver] Ignore -fmsc-version= -fms-compatibility-version= values smaller than 19

hans added a comment. In D134468#3812846 , @MaskRay wrote: > I am not a Windows driver user. This patch is entirely motivated by Windows > discussions in D131465 and I want to make > `/std:` default rule simple (say, c

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

hans added a comment. Well it has to adapt to MSVC's library files, so it can't always do the same regardless of version. Given that MSVC has supported /std:c++17 for a while, it's probably safe to bump the default, but perhaps not for all versions. Repository: rG LLVM Github Monorepo CHAN

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

hans added a comment. In D131465#3821701 , @aaron.ballman wrote: > Perhaps the simple rule we're going for is "when executed on Windows, Clang > defaults to C++17 unless it is executed from a context in which an MSVC > library can be detected, in which

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

hans added a comment. > Right, we did have a record layout fuzzer which worked pretty well. I think > it only ever ran on a workstation, it relied on custom tools and the like. I > think it's lost to the sands of time. (The code still exists: https://github.com/dberlin/superfuzz I've never run

[PATCH] D135154: Keep inherited dllimport/export attrs for explicit specialization of template class member functions

hans created this revision. hans added a reviewer: thakis. Herald added a subscriber: mstorsjo. Herald added a project: All. hans requested review of this revision. Herald added a project: clang. Previously we were stripping these normally inherited attributes during explicit specialization. Howe

[PATCH] D135154: Keep inherited dllimport/export attrs for explicit specialization of template class member functions

hans added a comment. This did turn up a problem. With this patch Clang treats the following (based on https://source.chromium.org/chromium/chromium/src/+/main:media/formats/hls/source_string.h;l=125) as invalid, and so does MSVC: template struct __declspec(dllexport) Foo { static Foo

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

hans accepted this revision. hans 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/D134456/new/ https://reviews.llvm.org/D134456 ___ cfe

[PATCH] D135154: Keep inherited dllimport/export attrs for explicit specialization of template class member functions

hans added a comment. Looking at the Chromium code some more, the case of trying to delete a specialization of a dllexport class template member seems suspect. Since the error also matches MSVC I think we should keep that behavior, but I'll add a release note about it. Repository: rG LLVM G

[PATCH] D135154: Keep inherited dllimport/export attrs for explicit specialization of template class member functions

This revision was automatically updated to reflect the committed changes. Closed by commit rGc9b771b9fc2f: Keep inherited dllimport/export attrs for explicit specialization of class… (authored by hans). Changed prior to commit: https://reviews.llvm.org/D135154?vs=464984&id=466023#toc Repositor

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

hans added a comment. We're hitting a false positive in grpc after this: > ../../third_party/grpc/src/src/core/lib/gprpp/ref_counted_ptr.h:335:31: error: calling function 'TlsSessionKeyLoggerCache' requires holding mutex 'g_tls_session_key_log_cache_mu' exclusively [-Werror,-Wthread-safety-an

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

hans added a comment. We also hit a different case: https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c6 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129755/new/ https://reviews.llvm.org/D129755 __

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

hans added a comment. In D129755#3843146 , @aaronpuchert wrote: > In D129755#3842729 , @hans wrote: > >> We're hitting a false positive in grpc after this: >> >> > ../../third_party/grpc/src/src/core/lib/gprpp/

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

hans added a comment. In D129755#3849540 , @aaron.ballman wrote: >>> In D129755#3842744 , @hans wrote: >>> >>> >>> I'll give you some time to reply before I reland, but I can't see a bug >>> here. For the

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

hans added a comment. In D129755#3850731 , @aaronpuchert wrote: > @hans, where you able to fix or work around the warnings? I'd like to land > this again, but if you need more time it can also wait a bit. The work-around for gRPC is still pending, but

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

hans added a comment. Heads up that we're seeing a problem in Chromium that bisects to this change. (http://crbug.com/1373836) In our case what's happening is that the compiler is getting the name of an included file wrong, which ends up in the debug info (and also shows in the preprocessor ou

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

hans added a comment. Here's a reproducer of what's happening in our case. It's using the preprocessor because that's the easiest, but the real problem for us is that the debug info is pointing to the wrong file (and that it's different depending on the symlinkedness of the include). $ cat /

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

hans added a comment. 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. Here's a new version of the reproducer that's a little less convoluted: $ echo 1, 2, 3 > /tmp/foo.h $ ln

[PATCH] D139938: [clang] Don't spuriously pass -stdlib=libc++ to CC1 on Darwin

hans added a comment. This seems to have broken the instrprof-darwin-exports.c test, see e.g. https://green.lab.llvm.org/green/job/clang-stage1-RA/32351/ I'll revert for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139938/new/ https://revie

[PATCH] D136846: [Driver] Add -fsample-profile-use-profi

hans accepted this revision. hans added a comment. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136846/new/ https://reviews.llvm.org/D136846 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D137322: [clang][pdb] Don't include -fmessage-length in PDB buildinfo

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm with nits Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:47 +// MESSAGELEN: 0x{{.+}} | LF_BUILDINFO [size = {{.+}}] +// MESSAGELEN-NOT: 0x{{.+}}: `"

[PATCH] D137269: [Clang][AArch64][Darwin] Enable GlobalISel by default for Darwin ARM64 platforms.

hans added a comment. Looks like there are a few issues flagged already. Is someone reverting, or what's the plan? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137269/new/ https://reviews.llvm.org/D137269

[PATCH] D142826: [Clang] Add -Wtype-limits to -Wextra for GCC compatibility

hans added a comment. In D142826#4098014 , @chapuni wrote: > This discovers a warning in > https://reviews.llvm.org/rGa68d4b11465f5b3326be1dd820f59fac275b7581 > I think its checking is valid if size_t is uint32_t (eg. -m32) > > Could you guys teach me wh

[PATCH] D142384: [C++20] Fix a crash with modules.

hans added a comment. It looks like this is making one of Chromium's clang plugins traverse (and flag) more code: https://bugs.chromium.org/p/chromium/issues/detail?id=1412769 No modules involved :) I don't know if that's expected or not, but maybe that behavior change could be used as an inspi

[PATCH] D142867: [Clang] Add machinery to catch overflow in unary minus outside of a constant expression context

hans added a comment. Amusingly this fired on some code that used negation with the intention of getting fewer warnings: https://github.com/KhronosGroup/glslang/commit/866f67140ed98865350b53312bba1b654b59f937 :-) Fixed in https://github.com/KhronosGroup/glslang/commit/7341a21b345e7aea1d2791db0

[PATCH] D127452: [clang-cl][MSVC] Map /external:Wn n=1-4 to -Wsystem-headers

hans added a comment. In D127452#3577729 , @steplong wrote: > Hmm do you know how I can restart the pre-merge checks? It looks like the x64 > debian openmp tests failed. I don't think this patch is related, but I want > to make sure You can rebase and

[PATCH] D127519: [clang-cl] Accept /FA[c][s][u], but ignore the arguments

hans added a comment. > Previously, /FAsc would emit a warning. Now, it will just do what /FA does. I suppose we could still warn that the 's' and 'c' parts are ignored.. though I suppose that becomes evident from the output file.. If we're going to treat them the same, how about just making th

[PATCH] D127519: [clang-cl] Accept /FA[c][s][u], but ignore the arguments

hans accepted this revision. hans 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/D127519/new/ https://reviews.llvm.org/D127519 ___ cfe

[PATCH] D127641: [clang-cl][MSVC] Add default /Zc conformance arguments

hans added inline comments. Comment at: clang/test/Driver/cl-zc.cpp:98 // thread safe statics are off for versions < 19. // RUN: %clang_cl /c -### -fms-compatibility-version=18 -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s // RUN: %clang_cl /Zc:threadSafeInit /Z

[PATCH] D127641: [clang-cl][MSVC] Enable /Zc:alignedNew for C++17 and /Zc:sizedDealloc by default

hans added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6642 // -fsized-deallocation is off by default, as it is an ABI-breaking change for - // most platforms. - Args.addOptInFlag(CmdArgs, options::OPT_fsized_deallocation, -options

[PATCH] D127641: [clang-cl][MSVC] Enable /Zc:alignedNew for C++17 and /Zc:sizedDealloc by default

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm (If you like, consider dropping the "-DEFAULT" part from the new filecheck prefixes. I don't think they really add much value.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D127641: [clang-cl][MSVC] Enable /Zc:alignedNew for C++17 and /Zc:sizedDealloc by default

hans added a comment. In D127641#3597360 , @steplong wrote: > It looks like misc-new-delete-overloads.cpp is failing on line 20: > .. > On Line 16, it says sized deallocations are not enabled by default, but this > patch turns it on by default for MSVC.

[PATCH] D126559: [MSVC] Fix pragma alloc_text failing for C files

hans added a comment. > Ignoring the `pragma alloc_text`, it looks like GCC compiles the following > `foo` with C linkage vs LLVM which compiles with C++ linkage (foo's name is > mangled): The mangled name shouldn't matter since it has internal linkage. I tried dropping the `static`, and the

[PATCH] D126559: [MSVC] Fix pragma alloc_text failing for C files

hans added a comment. > Oh I see, that makes sense. We aren't accepting > https://godbolt.org/z/9Yej9vhYd. Do you know of a way to get the `NamedDecl` > with `extern "C"` instead of the second declaration? I'm not sure I understand the question, but it seems the current code is checking `isExt

[PATCH] D128409: [clang-cl] Add -emit-ast to clang-cl driver

hans added a comment. I'm unfamiliar with -emit-ast. Can you add some background on what this is for? What's CTU? In any case this needs a test. Comment at: clang/include/clang/Driver/Options.td:834 def Xoffload_linker : JoinedAndSeparate<["-"], "Xoffload-linker">, - HelpTe

[PATCH] D128409: [clang-cl] Add -emit-ast to clang-cl driver

hans added a comment. In D128409#3604570 , @thieta wrote: > In D128409#3604460 , @hans wrote: > >> I'm unfamiliar with -emit-ast. Can you add some background on what this is >> for? What's CTU? > > CTU is cross t

[PATCH] D126397: [pseudo] Fix pseudo-gen usage when cross-compiling

hans added a comment. This didn't fix the mac/arm64 llvm builds for Chromium. I think it's because we don't set CMAKE_CROSSCOMPILING, so I'll try doing that as a work-around. (crbug.com/1330304). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D12639

[PATCH] D126664: Expand definition deprecation warning to include constexpr statements.

hans added a reviewer: aaron.ballman. hans added a comment. Looks reasonable to me. It would be good if Richard or Aaron could take a look too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126664/new/ https://reviews.llvm.org/D126664 ___

[PATCH] D126340: [clang][AIX] add option -mdefault-visibility-export-mapping

hans added a comment. We're hitting an assertion in Chromium due to this: llvm/clang/lib/AST/Decl.cpp:1510: clang::LinkageInfo clang::LinkageComputer::getLVForDecl(const clang::NamedDecl *, clang::LVComputationKind): Assertion `D->getCachedLinkage() == LV.getLinkage()' failed. See https://b

[PATCH] D127452: [clang-cl][MSVC] Map /external:Wn n=1-4 to -Wsystem-headers

hans added a comment. Right, this is not the same, but I suppose it's better than ignoring the flag. Comment at: clang/include/clang/Driver/Options.td:6462 Alias, AliasArgs<["no-invalid-source-encoding"]>; +def _SLASH_external_W0 : CLIgnoredFlag<"external:W0">; +def _SLASH_e

[PATCH] D127452: [clang-cl][MSVC] Map /external:Wn n=1-4 to -Wsystem-headers

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/include/clang/Driver/Options.td:6463 Alias, AliasArgs<["no-invalid-source-encoding"]>; +def _SLASH_external_W0 : CLFlag<"external:W0">, HelpText<"Ignore

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

hans added a comment. In D138846#4649147 , @alanphipps wrote: > In D138846#4649110 , @akhuang wrote: > >> I'm still working on a repro, but after this patch we're seeing "truncated >> profile data" errors in chr

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

hans added a comment. I just noticed this also broke some lit tests on mac: https://bugs.chromium.org/p/chromium/issues/detail?id=1485487#c0 That's also visible on greendragon: https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/35721/testReport/ Repository: rG LLVM Github Monore

[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

hans added a comment. We saw a new warning in Chromium after this, and I wasn't sure if that's intentional: #include template float foo(T input) { if (sizeof(T) > 2) { return 42; } else { constexpr float inverseMax = 1.0f / std::numeric_limits::max(); return in

[PATCH] D144006: [DebugMetadata][DwarfDebug] Support function-local types in lexical block scopes (4/7)

hans added a comment. We're hitting an assert after this change: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2331: virtual void llvm::DwarfDebug::endFunctionImpl(const llvm::MachineFunction *): Assertion `LScopes.getAbstractScopesList().size() == NumAbstractSubprograms && "getOrCreateAbstr

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

hans added a comment. In D157334#4573902 , @aaron.ballman wrote: > I'm curious to hear what others think about this (especially @rnk and @hans). Seems reasonable to me. (Looks like the define was originally added here: https://github.com/llvm/llvm-pro

[PATCH] D157808: [clang] Add missing field to VisibilityAttr json AST dump

hans added a comment. Since reverting seems non-trivial, I added a triple in https://github.com/llvm/llvm-project/commit/7a1735cd05c2bc0c336f122f01fb35de66e85e16 which I believe should fix the test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D15

[PATCH] D155713: [clang] Fix interaction between dllimport and exclude_from_explicit_instantiation

hans added a comment. Apologies for the delay, I'm still catching up with my post-vacation backlog. (For my notes, this came up in https://reviews.llvm.org/D153709) I think the root of this bug is how class-level dll attributes get propagated to class members. Here is an example: template

[PATCH] D155713: [clang] Fix interaction between dllimport and exclude_from_explicit_instantiation

hans added a comment. In D155713#4592667 , @rnk wrote: >> That doesn't handle the second of your test cases though, where dllimport is >> put on the member directly: >> ... >> It's not clear to me how we'd want to handle that. I don't think it comes up

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

hans added a comment. > Thanks! I agree. d9ad0681fad9a98f43d9baddb95d505b37153c48 (2013) renamed > `warn_drv_overriding_t_option` to `warn_drv_overriding_flag_option`. > Perhaps the original name `warn_drv_overriding_t_option` should be restored. That change also started using it for overriding

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

hans accepted this revision. hans added a comment. lgtm Maybe add a short release note. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158137/new/ https://reviews.llvm.org/D158137 ___ cfe-commits mailing

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

hans 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-TNO-OS

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

hans 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-TNO-OS

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

hans added a comment. Given the concerns raised (the PRIuS one in https://godbolt.org/z/v3boc9E6T seems like a good example), and that the fix in D158372 doesn't seem straight-forward, could this be reverted to unbreak things until a revised version is ready?

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

hans added a comment. In D153156#4607655 , @aaron.ballman wrote: > I agree this should be reverted from 17.x so we have more time to consider an > appropriate path forward. Supporting evidence that this will likely be > disruptive: > https://sourcegra

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

hans 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-TNO-OS

[PATCH] D151479: [clang] Use IgnoreParensSingleStep in more places

hans accepted this revision. hans 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/D151479/new/ https://reviews.llvm.org/D151479 ___ cfe

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

hans added a comment. For those following along, this was relanded in https://reviews.llvm.org/D154007 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137872/new/ https://reviews.llvm.org/D137872 ___ cfe-c

[PATCH] D157245: [clang/cxx-interop] Teach clang to ignore availability errors that come from CF_OPTIONS

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Maybe we want this for the release/17.x branch too? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157245/new/ https://reviews.llvm.org/D157245 _

[PATCH] D155713: [clang] Fix interaction between dllimport and exclude_from_explicit_instantiation

hans added a comment. Sent my diff along with tests based on Louis's patch as a pull request here: https://github.com/llvm/llvm-project/pull/65961 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155713/new/ https://reviews.llvm.org/D155713

[PATCH] D154417: [libc++] Disable tree invariant check in asserts mode

hans created this revision. hans added reviewers: var-const, ldionne. Herald added a project: All. hans requested review of this revision. Herald added a project: libc++. Herald added a subscriber: libcxx-commits. Herald added a reviewer: libc++. This is a follow-up to D153672

[PATCH] D154417: [libc++] Disable tree invariant check in asserts mode

hans added a comment. In D154417#4471141 , @alexfh wrote: > Given the comment in https://reviews.llvm.org/D153672#4468348, I think, this > should be fine to submit. Sounds good to me. Happy to follow up if libc++ maintainers have more input. Repositor

[PATCH] D154417: [libc++] Disable tree invariant check in asserts mode

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG1e35e93e30c2: [libc++] Disable tree invariant check in asserts mode (authored by hans). Repository: rG LLVM Github Mono

[PATCH] D154295: [Driver][MSVC] Support DWARF fission when using LTO on Windows

hans accepted this revision. hans added a comment. Looks reasonable to me too. It would be nice to have some documentation for this feature though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154295/new/ https://reviews.llvm.org/D154295 ___

[PATCH] D154295: [Driver][MSVC] Support DWARF fission when using LTO on Windows

hans added a comment. In D154295#4477165 , @HaohaiWen wrote: >> It would be nice to have some documentation for this feature though. > > This feature is same as Linux -gsplit-dwarf. Right, but it would be nice to document that clang-cl supports it (which

[PATCH] D154295: [Driver][MSVC] Support DWARF fission when using LTO on Windows

hans added a comment. In D154295#4482818 , @HaohaiWen wrote: > In D154295#4477203 , @hans wrote: > >> In D154295#4477165 , @HaohaiWen >> wrote: >> It would be nice t

[PATCH] D159083: Clang: Don't warn about unused private fields of types declared maybe_unused

hans created this revision. hans added reviewers: aaron.ballman, shafik. Herald added a project: All. hans requested review of this revision. Herald added a project: clang. The compiler should not warn on code such as: class [[maybe_unused]] MaybeUnusedClass {}; class C { MaybeUnusedClass

[PATCH] D159083: Clang: Don't warn about unused private fields of types declared maybe_unused

hans updated this revision to Diff 554312. hans added a comment. Add release note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159083/new/ https://reviews.llvm.org/D159083 Files: clang/docs/ReleaseNotes.rst clang/lib/Sema/SemaDeclCXX.cpp clang/test/SemaCXX/warn-unused-private-f

[PATCH] D159083: Clang: Don't warn about unused private fields of types declared maybe_unused

hans added inline comments. Comment at: clang/test/SemaCXX/warn-unused-private-field.cpp:291-292 +enum [[maybe_unused]] MaybeUnusedEnum {}; +typedef int MaybeUnusedTypedef [[maybe_unused]]; +class C { + MaybeUnusedClass c; // no-warning aaron.ballman wrote: > Le

[PATCH] D159083: Clang: Don't warn about unused private fields of types declared maybe_unused

hans added inline comments. Comment at: clang/test/SemaCXX/warn-unused-private-field.cpp:291-292 +enum [[maybe_unused]] MaybeUnusedEnum {}; +typedef int MaybeUnusedTypedef [[maybe_unused]]; +class C { + MaybeUnusedClass c; // no-warning aaron.ballman wrote: > ha

[PATCH] D159083: Clang: Don't warn about unused private fields of types declared maybe_unused

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa4fbc0918462: Clang: Don't warn about unused private fields of types declared maybe_unused (authored by hans). Changed prior to commit: https://re

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

hans added a comment. In D152495#4628785 , @cor3ntin wrote: > It is used, but only in an assert. Seems like the right fix! I suppose it is technically, but I'm not sure the fix reads like an improvement to me... I guess that's often the case with unused

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

hans added a comment. In D152495#4628887 , @aaron.ballman wrote: > That might not be a bad idea in this case -- perhaps > `-Wunused-condition-variable` and group it under `-Wunused-variable`? Sounds god to me conceptually. (I don't know the code, so do

[PATCH] D153924: [OpenMP] Allow exceptions in target regions when offloading to GPUs

hans added inline comments. Comment at: clang/test/OpenMP/amdgpu_exceptions.cpp:10 + +// RUN: %clang_cc1 -fopenmp -triple amdgcn-amd-amdhsa -fopenmp-is-target-device -fcxx-exceptions -fexceptions %s -emit-llvm -S -verify=with -Wopenmp-target-exception -analyze +// RUN: %clang_c

[PATCH] D138157: Make -fsanitize=scudo use scudo_standalone. Delete check-scudo.

hans added a comment. Chromium is broken too CMake Error at /b/s/w/ir/cache/builder/src/third_party/llvm/llvm/cmake/modules/AddLLVM.cmake:1915 (add_dependencies): The dependency target "ScudoUnitTests" of target "check-runtimes" does not exist. Call Stack (most recent call first):

[PATCH] D138157: Make -fsanitize=scudo use scudo_standalone. Delete check-scudo.

hans added a comment. Reverted in 907baeec49bfbe9e76498634a9418e1dc6c973d9 for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138157/new/ https://reviews.llvm.org/D138157 ___ cfe-commits mailing list

[PATCH] D139167: [clang][Windows]Ignore Options '/d1nodatetime' and '/d1import_no_registry'

hans added a comment. I hadn't seen `/d1` flags before. I suppose they relate to preprocessor stuff as opposed to `/d2` flags. I think the "Unsupported" section below may be better for these. In that case clang-cl will parse the option and warn that it's ignoring it. On line 6931 there's a cat

[PATCH] D139167: [clang][Windows]Ignore Options '/d1nodatetime' and '/d1import_no_registry'

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks great, thanks! Do you have commit access, or would you like someone to commit this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1391

[PATCH] D76030: [CUDA] Warn about unsupported CUDA SDK version only if it's used.

hans added a comment. In D76030#1928448 , @hans wrote: > In D76030#1925445 , @tra wrote: > > > @hans -- this should be cherry-picked into 10 if it's not too late yet. > > > It's probably too late for 10.0.0, but so

[PATCH] D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

hans created this revision. hans added reviewers: sammccall, MyDeveloperDay, thakis. MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. LGTM This fixes a regression from D69573 which broke the

[PATCH] D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

hans marked an inline comment as done. hans added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:2806 + if (Previous->is(tok::identifier) || Previous->isSimpleTypeSpecifier()) { +Previous = Previous->Previous; +continue; kra

[PATCH] D76850: clang-format: Fix pointer alignment for overloaded operators (PR45107)

This revision was automatically updated to reflect the committed changes. Closed by commit rGeb85e90350e9: clang-format: Fix pointer alignment for overloaded operators (PR45107) (authored by hans). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D76850?vs=252842

[PATCH] D74878: [remark][diagnostics] [codegen] Fix PR44896

hans added a comment. See also https://reviews.llvm.org/D74871 I don't know which is the better fix here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74878/new/ https://reviews.llvm.org/D74878 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D74871: Fix interaction between -fdiscard-value-names and LLVM Bitcode

hans added a reviewer: xur. hans added a comment. Thanks for looking into this, Serge! I used your test case to bisect where the crashing started, and ended up at 60d39479221d6bc09060f7816bcd7c54eb286603 Someone else noted t

[PATCH] D75047: Add Control Flow Guard in Clang release notes.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Excellent. Thanks! Do you have commits access, or would you like me to commit for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75047/new/ htt

[PATCH] D75093: clang-cl: Add a `/showIncludes:user` flag.

hans added a comment. Instead of replacing the old cc1 "--show-includes" with "--show-includes -sys-header-deps" -- which looks a little ugly maybe -- would it be simpler to just introduce a new cc1 flag instead, e.g. "--show-user-includes", and just expand /showIncludes:user to that? CHANGES

[PATCH] D75047: Add Control Flow Guard in Clang release notes.

hans added a comment. Pushed to 10.x as 3a11c86849c27e1e21d5e8cdf55cfa724164db57 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75047/new/ https://reviews.llvm.org/D75047

[PATCH] D75012: [ReleaseNotes] Mention -fmacro-prefix-map and -ffile-prefix-map.

hans added a comment. > Should such change be committed directly to the release/10.x branch by @hans ? Peter can commit to release/10.x himself, or let me know if he prefers me to do it. Thanks for writing release notes! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

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