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

hans added a comment. > I just saw @glandium's earlier comment: > >> Code built with older versions of LLVM (e.g. rust) and running with the >> updated runtime crash at startup with this change. > > This is the exact same issue we encountered. Because there is a profile > format change, it's exp

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

hans added a comment. We're seeing crashes in `initializeValueProfRuntimeRecord` that bisects to this commit. I think Zequan is investigating: https://bugs.chromium.org/p/chromium/issues/detail?id=1503919 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

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

hans added a comment. In D138846#4656594 , @hans wrote: > Just a heads up that we're seeing crashes in the Rust compiler which bisects > to this change. Investigating in > https://bugs.chromium.org/p/chromium/issues/detail?id=1500558 Here's a small rep

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

hans added a comment. Just a heads up that we're seeing crashes in the Rust compiler which bisects to this change. Investigating in https://bugs.chromium.org/p/chromium/issues/detail?id=1500558 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138846/

[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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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

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 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 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] 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. 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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

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] D147875: [clang][Diagnostics] Show line numbers when printing code snippets

hans added a comment. I noticed that at least for some cases of `-Wformat`, the line numbers on the left seem to be off: https://github.com/llvm/llvm-project/issues/63524 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147875/new/ https://reviews.ll

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

hans added a comment. We're seeing assertion failures in Chromium too. Reproducer for x86_64 Linux here: https://bugs.chromium.org/p/chromium/issues/detail?id=1456288#c2 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144006/new/ https://reviews.llv

[PATCH] D153176: [Frontend] Remove ShowIncludesPretendHeader

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/D153176/new/ https://reviews.llvm.org/D153176 ___ cfe

[PATCH] D153175: [Frontend] Don't output skipped includes from predefines

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. > -H is supposed to skip outputting headers from -include command line > arguments, but -fshow-skipped-includes was outputting any skipped > includes encountered via -include. I was thrown off by t

[PATCH] D152696: Prevent deadlocks in death tests.

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/D152696/new/ https://reviews.llvm.org/D152696 ___ cfe

[PATCH] D152696: Prevent deadlocks in death tests.

hans added a comment. I'm no expert here, but I went to check what Chromium does, and it seems to set the death_test_style to "threadsafe" for the whole test suite: https://source.chromium.org/chromium/chromium/src/+/main:base/test/test_suite.cc;l=367 > As the page linked above notes, "flags ar

[PATCH] D152090: [clang][Driver] Add -fcaret-diagnostics-max-lines as a driver option

hans added a comment. Should there be a test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152090/new/ https://reviews.llvm.org/D152090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D147875: [clang][Diagnostics] Show line numbers when printing code snippets

hans added inline comments. Comment at: clang/docs/UsersManual.rst:578 + +.. option:: -fcaret-diagnostics-max-lines: + I think this is still a cc1-only option. Should it be made available as a driver option now? Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D151344: Reland "[CMake] Bumps minimum version to 3.20.0.

hans added a comment. This looks right to me. (I'm out of office at the moment, but this looks like what I tested in https://github.com/llvm/llvm-project/issues/62719 so it should work.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151344/new/ h

[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

hans added a comment. This broke the tests on Mac, see e.g. https://green.lab.llvm.org/green/job/clang-stage1-RA/34396/consoleFull (or http://crbug.com/1447928) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148785/new/ https://reviews.llvm.org/D1

[PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

hans added a comment. >>> Do you have a suggestion how we can move this patch forward? >> >> IIRC, D150688 + the diff in >> https://github.com/llvm/llvm-project/issues/62719#issuecomment-1552903385 + >> upgrading the pre-merge linux bot should take care of all

[PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

hans added a comment. In D144509#4350052 , @Mordante wrote: > In D144509#4349921 , @thakis wrote: > >> Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 >>

[PATCH] D150817: Use windows baskslash on anonymous tag locations if using MSVCFormatting and it's not absolute path.

hans accepted this revision. hans added a comment. Nice! lgtm Comment at: clang/lib/AST/TypePrinter.cpp:1391 + WrittenFile = Callbacks->remapPath(File); +// The following tries to fix inconsistent path separator created by +// clang::DirectoryLookup::Lo

[PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

hans added a comment. In D144509#4349399 , @mstorsjo wrote: > In D144509#4349051 , @hans wrote: > >> In D144509#4347562 , @glandium >> wrote: >> >>> FYI, 65429b9af6a2c99d

[PATCH] D144509: [CMake] Bumps minimum version to 3.20.0.

hans added a comment. In D144509#4347562 , @glandium wrote: > FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c > is > causing problems on Windows compiler-rt for some reason I haven't id

[PATCH] D149999: [clang-tidy] [test] Narrow down a special case to MSVC mode

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/D14/new/ https://reviews.llvm.org/D14 ___ cfe

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

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/D149997/new/ https://reviews.llvm.org/D149997 ___ cfe

[PATCH] D150001: [clang] Fix initializer_list matching failures with modules

hans added a comment. Nice! Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11570 getStdNamespace()->setImplicit(true); +Context.getTranslationUnitDecl()->addDecl(getStdNamespace()); +getStdNamespace()->clearIdentifierNamespace(); This could use an expl

[PATCH] D149695: MS inline asm: remove obsolete code adding AOK_SizeDirective (e.g. dword ptr)

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/D149695/new/ https://reviews.llvm.org/D149695 ___ cfe

[PATCH] D149695: MS inline asm: remove obsolete code adding AOK_SizeDirective (e.g. dword ptr)

hans added a comment. > The AOK_SizeDirective part from 5b37c181291210bedfbb7a6af5d51229f3652ef0 > (2014-08) seems unneeded nowadays (the root cause has likely been fixed > elsewhere). Would it be possible to confirm when/if the size directive here became unnecessary? Repository: rG LLVM Git

[PATCH] D149206: [clang][driver] Enable MisExpect diagnostics flag outside of CC1

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

[PATCH] D149206: [clang][driver] Enable MisExpect diagnostics flag outside of CC1

hans added inline comments. Comment at: clang/test/Driver/clang_f_opts.c:144 +// RUN: %clang -### -S -fprofile-instr-use=%t.profdata -fdiagnostics-misexpect-tolerance=10 -Wmisexpect %s 2>&1 | FileCheck %s --check-prefix=CHECK-MISEXPECT-TOLLERANCE +// CHECK-MISEXPECT-TOLLERANCE-

[PATCH] D149206: [clang][driver] Enable MisExpect diagnostics flag outside of CC1

hans added inline comments. Comment at: clang/test/Profile/misexpect-branch.c:10 // RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify=foo -fdiagnostics-misexpect-tolerance=10 -Wmisexpect -debug-info-kind=line-tables-only +// RUN: %clang

[PATCH] D149187: [clang] Canonicalize system headers in dependency file when -canonical-prefixes

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Thanks! I like it. Comment at: clang/include/clang/Frontend/Utils.h:44 class Preprocessor; +class FileManager; class PreprocessorOptions; nit: sort? Reposito

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

hans added a comment. In D115907#4295923 , @paulkirth wrote: >> 2. Due to inlining etc., it often gets the source locations wrong, which >> means it points at code where again there were no expectations -- but >> perhaps that code got inlined into an ex

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

hans added a comment. Herald added subscribers: hoy, Enna1, StephenFan. We gave this a try in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1434989 While the diagnostics are interesting, two things make this less useful for our developers: 1. It also fires in situations where

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/Driver/ToolChains/Clang.cpp:583 + llvm::sys::path::Style Style = + llvm::sys::path::is_absolute(ObjFileNameForDebug) + ? llvm::sys::path

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:583 + llvm::sys::path::Style Style = + llvm::sys::path::is_absolute(ObjFileNameForDebug) + ? llvm::sys::path::Style::native zequanwu wrote: > hans wrote: > > Won't the co

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:583 + llvm::sys::path::Style Style = + llvm::sys::path::is_absolute(ObjFileNameForDebug) + ? llvm::sys::path::Style::native Won't the code above (line 580) make many file

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans added a comment. In D147256#4262002 , @zequanwu wrote: > In D147256#4261949 , @hans wrote: > >>> Well, MSVC cl removes redundant dots so we shouldn't remove >>> llvm::sys::path::remove_dots. >> >> Could we d

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans added a comment. > Well, MSVC cl removes redundant dots so we shouldn't remove > llvm::sys::path::remove_dots. Could we do the `remove_dots` on the Clang side, where we can decide based on the LangOpts? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D147073: [Coverage] Handle invalid end location of an expression/statement.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Again not an expert here, but lgtm. (Nit: the https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaExprCXX.cpp#L1528-L1530 link in the description seems to point to the wrong code no

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans added a comment. In D147256#4249527 , @zequanwu wrote: > - Add a `-use-target-path-separator` flag for llc. > - Add test for llc with that flag. But where does `TM.Options.ObjectFilenameForDebug` come from? Presumably it comes from Clang at some po

[PATCH] D147073: [Coverage] Handle invalid end location of an expression/statement.

hans added a comment. In D147073#4240793 , @zequanwu wrote: > In D147073#4240017 , @hans wrote: > >> I'm not familiar with this code. I suppose the question is whether it's >> reasonable for this code to expect t

[PATCH] D147073: [Coverage] Handle invalid end location of an expression/statement.

hans added a comment. I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not? Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:605 +// If the either location is inv

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans added a comment. In D147256#4237099 , @probinson wrote: > I think we cannot be 100% sure about source paths in a cross-compile > situation. Cross-compiling on platform A targeting platform B does not mean > your sources and debugger UI are on platf

[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

hans added a comment. Thanks for working on this! The `-ffile-reproducible` flag name refers to making `#file` directives reproducible, but `LangOptions.UseTargetPathSeparator` sounds a lot broader :) I don't know what others think, but it would be nice to not have to introduce any more flags

[PATCH] D146338: [MSVC compatibility][dllimport/dllexport][PS] Allow dllexport/dllimport for classes with UniqueExternalLinkage

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/D146338/new/ https://reviews.llvm.org/D146338 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D146764: [clang] Make predefined expressions string literals under -fms-extensions

hans added a reviewer: mstorsjo. hans added a comment. +mstorsjo is this okay for mingw mode too? Comment at: clang/lib/Sema/SemaExpr.cpp:3558 +return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL); + } else { // Pre-defined identifiers are of type char[x], where

[PATCH] D146338: [MSVC compatibility][dllimport/dllexport][PS] Allow dllexport/dllimport for classes with UniqueExternalLinkage

hans added a comment. Thanks, I like this. Could you please also add tests in CodeGenCXX/dll{ex,im}port.cpp which verify that the IR looks right? Comment at: clang/test/SemaCXX/dllexport.cpp:437 +class Base {}; +class __declspec(dllexport) ExportedClass {};

[PATCH] D146165: docs: add some documentation on Windows SDK search

hans added a comment. Nice, thanks for taking the time to expand on this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146165/new/ https://reviews.llvm.org/D146165 ___ cfe-commits mailing list cfe-commi

[PATCH] D146490: [Support] On Windows, ensure that UniqueID is really stable

hans added a comment. This makes me nervous as well. I think it also doesn't fit well with developers' expectations of UniqueID, changing it from a simple piece of data to something which interacts with the OS in a pretty deep way. I don't have specific examples, but I'm sure it will bite us so

[PATCH] D146165: docs: add some documentation on Windows SDK search

hans accepted this revision. hans added inline comments. This revision is now accepted and ready to land. Comment at: clang/docs/UsersManual.rst:4504 +programs against the Windows system packages. Underlying the Windows SDK is the +UCRT, the universal C runtime. + -

[PATCH] D146338: [MSVC compatibility][dllimport/dllexport][PS] Allow dllexport/dllimport for classes with UniqueExternalLinkage

hans added a comment. > In D145271 it was suggested that we drop > the attribute in such contexts, and this is effectively what happens. The > compiler does not produce any exported definitions (or import any symbols) > for such classes. The patch is simply to

[PATCH] D146280: [clang] Include the error message in file reading error diagnostic

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe495eabd3268: [clang] Include the error message in file reading error diagnostic (authored by hans). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D146280: [clang] Include the error message in file reading error diagnostic

hans added a comment. In D146280#4201769 , @thakis wrote: > (if the presubmit is related, maybe some flang test needs updating? Weird > that flang uses clang's diags.) The presubmit errors look unrelated and the flang tests seem fine locally. Reposito

[PATCH] D146280: [clang] Include the error message in file reading error diagnostic

hans created this revision. hans added reviewers: aaron.ballman, MaskRay. Herald added projects: Flang, All. hans requested review of this revision. Herald added a subscriber: jdoerfert. Herald added a project: clang. in order to provide as much information as possible to the user. The diagnostic

[PATCH] D146211: Make globals used for array initialization codegen constant

This revision was automatically updated to reflect the committed changes. Closed by commit rG4a2757d80f0a: Make globals used for array initialization codegen constant (authored by hans). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146211/new/ http

[PATCH] D146165: docs: add some documentation on Windows SDK search

hans added a comment. Nice, thanks for doing this! Comment at: clang/docs/UsersManual.rst:4537 + +TODO: This is not yet implemented. + But isn't this how clang-cl finds stuff when running in a "Visual Studio Command Prompt" / setenv / vcvars or whatever it

[PATCH] D146164: Fix nomerge attribute not working with __builtin_trap().

hans added a comment. Since we're touching SelectionDAG, does GlobalISel also need updating? Comment at: clang/test/CodeGen/attr-nomerge.cpp:44 + + [[clang::nomerge]] __builtin_trap(); } Maybe do __debugbreak() too since that's also mentioned on the debug.

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

hans added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:535 CGM.getModule(), C->getType(), - CGM.isTypeConstant(ArrayQTy, /* ExcludeCtorDtor= */ true), + CGM.isTypeConstant(ArrayQTy, /* ExcludeCtor= */ true, +

[PATCH] D146211: Make globals used for array initialization codegen constant

hans created this revision. hans added a reviewer: efriedma. Herald added a project: All. hans requested review of this revision. Herald added a project: clang. As pointed out in D133835 these globals will never be written to (they're only used for trivially cop

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

This revision was automatically updated to reflect the committed changes. hans marked an inline comment as done. Closed by commit rG7a85aa918ccd: Emit const globals with constexpr destructor as constant LLVM values (authored by hans). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

hans updated this revision to Diff 505466. hans marked an inline comment as done. hans added a comment. Passing a `ExcludeDtor` param. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145369/new/ https://reviews.llvm.org/D145369 Files: clang/lib/CodeGen/CGDecl.cpp clang/lib/CodeGen/CG

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

hans marked an inline comment as done. hans added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4344 + (Record->hasTrivialDestructor() || + Record->hasConstexprDestructor()); } efriedma wrote: > For the purposes of C

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

hans updated this revision to Diff 505127. hans added a comment. Thanks for the review! Added tests to cover other calls that change behavior due to this. Please take another look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145369/new/ https://reviews.llvm.org/D145369 Files: cla

[PATCH] D145970: [MSVC][PS][dllexport/dllimport] Propagate a dllexport/dllimport attribute to template baseclasses

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. LGTM I've always found this to be an interesting behavior, and I'm guessing you found some code where it does matter :) Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2612 + if (C

[PATCH] D145951: [docs] Add more complete documentation for -f[no]split-lto-unit

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. LGTM, thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145951/new/ https://reviews.llvm.org/D145951 ___

[PATCH] D145919: [libc++] Add missing include in exception_ptr.h

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc341d5030503: [libc++] Add missing include in exception_ptr.h (authored by hans). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D145919: [libc++] Add missing include in exception_ptr.h

hans created this revision. hans added a reviewer: philnik. hans added a project: libc++. Herald added a project: All. hans requested review of this revision. Herald added a subscriber: libcxx-commits. Herald added a reviewer: libc++. Windows builds were failing due to missing include for std::add

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

hans added a comment. In D145369#4181296 , @ilya-biryukov wrote: > I don't have a lot of experience in codegen, so will let Aaron and Richard do > the review. Thanks for checking! I don't have a lot of experience here either, so any review is much app

[PATCH] D145517: MSVC: support version preference with search

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm I wish the way clang-cl finds system libraries was better documented though. Since you've been digging through this code, would you be up for writing something in the https://clang.llvm.org/

[PATCH] D145271: [MSVC compatibility][DLLEXPORT/DLLIMPORT] Allow dllexport/dllimport for local classes

hans added a comment. In D145271#4178837 , @wolfgangp wrote: >> It still seems like the export/import there is an accident since >> `Base` can't really be referenced from outside the file anyway. >> >> Perhaps rather than giving `Base` external linkage t

  1   2   3   4   5   6   7   8   9   10   >