[PATCH] D72769: Replace CLANG_SPAWN_CC1 env var with a driver mode flag

hans added a comment. Oh, and removed the release note from trunk in 00c74d0b644 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72769/new/ https://reviews.llvm.org/D72769 ___

[PATCH] D72769: Replace CLANG_SPAWN_CC1 env var with a driver mode flag

hans added a comment. In D72769#1823488 , @hans wrote: > In D72769#1823473 , @sammccall wrote: > > > As I understand, the original patch made the new behaviour on-by-default > > and will make the release, so we wan

[PATCH] D72848: Remove some SVN-specific code.

hans added inline comments. Comment at: clang/lib/Basic/Version.cpp:33 #else - StringRef URL(""); + return ""; #endif Eugene.Zelenko wrote: > return {} should be better. Why? I think "" is clearer for a string. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

hans added inline comments. Comment at: clang/tools/driver/driver.cpp:338 + static unsigned ReenteranceCount; + if (ReenteranceCount++) +llvm::cl::ResetAllOptionOccurrences(); This looks pretty hacky. Can you explain more what the current problem is and ho

[PATCH] D72982: [Clang] Un-break scan-build after integrated-cc1 change

hans added a comment. Wait, do we really want the "(in-process)" marker to be written to a separate line? I'm not sure that we do. The change description doesn't explain how scan-build was broken or how this fixes it. And, maybe most importantly, it seems scan-build doesn't have any tests that

[PATCH] D73108: [docs][mips] 10.0 Release notes

hans added a comment. Thanks for writing release notes! Go ahead and push directly to the release/10.x branch when you're ready. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73108/new/ https://reviews.llvm.org/D73108 __

[PATCH] D72793: [clang-format] Expand the SpacesAroundConditions option to include catch statements

hans added a comment. In D72793#1828870 , @MyDeveloperDay wrote: > In D72793#1828625 , @timwoj wrote: > > > I know that 10.0 was branch recently. Any chance this can make it over? > > > I don't personally ever do t

[PATCH] D72903: [HIP] use GetProgramPath for executable discovery

hans added a comment. In D72903#1831716 , @tra wrote: > I've landed the change. As for backporting, I'll leave it up to AMD folks. If > @yaxunl and the team deem it appropriate, they should talk to @hans about > cherry-picking the change into particular

[PATCH] D72848: Remove some SVN-specific code.

hans added inline comments. Comment at: clang/lib/Basic/Version.cpp:33 #else - StringRef URL(""); + return ""; #endif Eugene.Zelenko wrote: > hans wrote: > > Eugene.Zelenko wrote: > > > return {} should be better. > > Why? I think "" is clearer for a string.

[PATCH] D72982: [Clang] Un-break scan-build after integrated-cc1 change

hans added a comment. In D72982#1831648 , @xazax.hun wrote: > In D72982#1831595 , @hans wrote: > > > Wait, do we really want the "(in-process)" marker to be written to a > > separate line? I'm not sure that we do.

[PATCH] D73060: [Clang] Fix expansion of response files in -Wp after integrated-cc1 change

hans added inline comments. Comment at: clang/tools/driver/driver.cpp:338 + static unsigned ReenteranceCount; + if (ReenteranceCount++) +llvm::cl::ResetAllOptionOccurrences(); aganea wrote: > hans wrote: > > This looks pretty hacky. > > > > Can you explain

[PATCH] D72982: [Clang] Un-break scan-build after integrated-cc1 change

hans added a comment. > Due to some unfortunate historical reasons fixing scan-build means fixing 3 > separate code bases. We have the original scan-build written in perl, a > python rewrite both checked-in in the LLVM repository, and on github. And the > two versions are not the same as the au

[PATCH] D72903: [HIP] use GetProgramPath for executable discovery

hans added a comment. In D72903#1832070 , @yaxunl wrote: > Sorry for the delay. I have no objection to cherrypick this change to release > 10. Thanks. I've cherry-picked it as 3cce3790072249cbe51b96cea26bc78019c11fd0

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

hans added a comment. In D72703#1833018 , @kimgr wrote: > I just want to say that finding the correlation between token count and > compile time is a bit of a breakthrough! I assume the same correlation could also be found with lines of code, but I thi

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

hans added a comment. In D72703#1832678 , @rnk wrote: > I waited to see if there was any other feedback, but I'm in favor of this. > > Should we try to come up with better pragma names? `clang max_tokens` doesn't > seem to call to mind what it does: warn

[PATCH] D72982: [Clang] Un-break scan-build after integrated-cc1 change

hans added a comment. In D72982#1832462 , @aganea wrote: > In D72982#1832233 , @aganea wrote: > > > In D72982#1832209 , @phosek wrote: > > > > > We're seeing `Driver/cc-print

[PATCH] D73120: [Clang] Alternate fix for "expansion of response files in -Wp after integrated-cc1 change"

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks reasonable to me. Comment at: clang/test/Driver/Wp-args.c:27 + +// RSP: inception Why inception? I feel like I'm missing something :) CHANGES SINCE LAST

[PATCH] D73120: [Clang] Alternate fix for "expansion of response files in -Wp after integrated-cc1 change"

hans added inline comments. Comment at: clang/test/Driver/Wp-args.c:27 + +// RSP: inception aganea wrote: > hans wrote: > > Why inception? I feel like I'm missing something :) > Could be anything else :) Because `-rewrite-macros` prints this .c with > comments,

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

hans added a comment. Cherry-picked to 10.x in 1f98c2b586e4ebce68d75856699059663a052cb7 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71980/new/ https://reviews.llvm.org/D71980

[PATCH] D73120: [Clang] Alternate fix for "expansion of response files in -Wp after integrated-cc1 change"

hans added a comment. Cherry-picked to 10.x in 85ee70e86456e3bcb3c706c404db497c5a448602 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73120/new/ https://reviews.llvm.org/D73120

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

hans added a comment. In D7#1836643 , @peter.smith wrote: > >> If the patchable functions is intended for clang-10 we'll need to make > >> sure any fix is merged to clang-10. > > > > This commit was made before release/10.x branch. Maybe the easiest

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

hans updated this revision to Diff 239944. hans added a comment. Doing max_tokens_here / max_tokens_total. Please take another look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72703/new/ https://reviews.llvm.org/D72703 Files: clang/include/clang/Basic/DiagnosticGroups.td clang/

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

hans added a comment. I've cherry-picked this to 10.x in 318677e78def0023d210a29f4b3cf648e02f9fdc . Please let me know if there are any follow-ups. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

hans added a comment. I've cherry-picked the revert (edd4398f4cd33a305afbca76ac4e6590e9337f4d ) to the 10.x branch in b079266dcb6d1ee6446d074ebd1d212a13ce0665

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

hans added a comment. In D69825#1837243 , @hctim wrote: > For tracking purposes. > > The ASan leaks have been worked around in two separate patches in order to > make the bots green again: > > - c38e42527b21acee8d01a016d5bfa2fb83202e29 >

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

hans added a comment. > @hans To address the CI problem (https://reviews.llvm.org/D7#1836728) > reported by @peter.smith, we just need to delete the driver option in the > release/10.x branch. (I need a -E preproccessed file to investigate.) > I think we don't need to hurry. We can simply r

[PATCH] D72630: [clang-tidy] Ignore implicit casts in modernize-use-default-member-init

hans added a comment. Ah, you're right, it's there. The reason for my comment was http://lists.llvm.org/pipermail/llvm-branch-commits/2020-January/013555.html but it seems it was also successfully pushed to master. Sorry for the noise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

hans added a comment. In D73007#1837621 , @rtrieu wrote: > I'm in favor of splitting the warning into subgroups, then deciding which > ones should be in -Wall. We've done this with other warnings such as the > conversion warnings and tautological compar

[PATCH] D73237: [CUDA] Fix order of memcpy arguments in __shfl_*(<64-bit type>).

hans added a comment. In D73237#1837077 , @tra wrote: > Landed in > https://github.com/llvm/llvm-project/commit/cc14de88da27a8178976972bdc8211c31f7ca9ae > @hans -- can we cherry-pick it into 10? Yes, go ahead and "git cherry-pick -x" it and push to the

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

hans added a comment. > FWIW, I'm not ecstatic about `max_tokens_here`, I thought `max_tokens_lexed` > had a nicer ring to it. /peanut. I mainly like the clarity in the difference between _here and _total. With _lexed, I feel that would not be as clear. CHANGES SINCE LAST ACTION https://rev

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

This revision was automatically updated to reflect the committed changes. Closed by commit rG739b410f1ff5: Add a warning, flags and pragmas to limit the number of pre-processor tokens in… (authored by hans). Herald added a project: clang. Changed prior to commit: https://reviews.llvm.org/D72703

[PATCH] D73231: [CUDA] Assume the latest known CUDA version if we've found an unknown one.

hans added a comment. In D73231#1845096 , @tra wrote: > @hans : that's another candidate for 10.x cherry-pick, if you're OK with it. Sounds good to me. Cherry-picked in 5777899f146aaa53f598e6978702a7a2725200d7

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

hans added a comment. > I created D73680 to place the patch label > after BTI. > > @hans Is there still time to cherry pick the patch to release/10.x? See > above, Linux developers really want the Clang release to have compatible > behavior with GCC. Yes, t

[PATCH] D45014: [Index] Return SourceLocation to consumers, not FileID/Offset pair.

hans added a comment. Some bots are sad: http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/16346/ http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/17364/ http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/28078/ Repository: rC Clan

[PATCH] D45165: Use llvm::sys::fs::real_path() in clang.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D45165 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D39331: Add flags to control the -finstrument-functions instrumentation calls to __cyg_profile functions

hans created this revision. This adds two flags: -fno-cygprofile-exit Don't insert any calls for function exit. -fno-cygprofile-args Don't pass any arguments to the cygprofile functions (the implementer has to figure out the caller address themselves) These are useful for reducing the size and

[PATCH] D39331: Add flags to control the -finstrument-functions instrumentation calls to __cyg_profile functions

hans added a comment. The request for -fno-cygprofile-args was from another user. You raise a good point, it sounds like mcount() might be perfect for them :-) https://reviews.llvm.org/D39331 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39579: BuiltinOperatorOverloadBuilder: Don't consider types that are unavailable on the target (PR35174)

hans created this revision. In the test case, we'd end up in a situation where Clang tried to mangle the __float128 types, which isn't supported when targeting MSVC, because it instantiated a variable template with that type when searching for a conversion. https://reviews.llvm.org/D39579 Fil

[PATCH] D39579: BuiltinOperatorOverloadBuilder: Don't consider types that are unavailable on the target (PR35174)

hans marked 2 inline comments as done. hans added inline comments. Comment at: test/CodeGenCXX/microsoft-cannot-mangle-float128.cpp:3-11 +template struct B; +template constexpr bool is_floating_point_v = false; + +struct StrictNumeric { + StrictNumeric(int); + template > = nu

[PATCH] D39579: BuiltinOperatorOverloadBuilder: Don't consider types that are unavailable on the target (PR35174)

hans updated this revision to Diff 121824. hans edited the summary of this revision. hans added a comment. Addressing comments and changing the test to exercise Sema more directly. Please take another look. https://reviews.llvm.org/D39579 Files: lib/Sema/SemaOverload.cpp test/SemaCXX/micro

[PATCH] D39331: Add flags to control the -finstrument-functions instrumentation calls to __cyg_profile functions

hans updated this revision to Diff 122557. hans added a comment. Updating to match the LLVM-side: https://reviews.llvm.org/D39287#inline-348143 https://reviews.llvm.org/D39331 Files: include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.def lib/CodeGen/CodeGenFunction.cpp

[PATCH] D39331: Add flags to control the -finstrument-functions instrumentation calls to __cyg_profile functions

hans added inline comments. Comment at: include/clang/Driver/Options.td:1032 +def fno_instrument_functions_inline : Flag<["-"], "fno-instrument-functions-inline">, Group, Flags<[CC1Option]>, + HelpText<"When using -finstrument-functions, insert the calls after inlining">; --

[PATCH] D39331: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes

hans updated this revision to Diff 122878. hans retitled this revision from "Add flags to control the -finstrument-functions instrumentation calls to __cyg_profile functions" to "Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes". hans edited the summary of this

[PATCH] D39331: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes

hans added inline comments. Comment at: include/clang/Driver/Options.td:1032 +def fno_instrument_functions_inline : Flag<["-"], "fno-instrument-functions-inline">, Group, Flags<[CC1Option]>, + HelpText<"When using -finstrument-functions, insert the calls after inlining">; --

[PATCH] D39331: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes

hans updated this revision to Diff 122889. hans edited the summary of this revision. hans added a comment. Renaming the new flag and making it sufficient to turn on instrumentation by itself. https://reviews.llvm.org/D39331 Files: include/clang/Driver/Options.td include/clang/Frontend/Code

[PATCH] D39331: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter attributes

This revision was automatically updated to reflect the committed changes. Closed by commit rL318199: Switch -mcount and -finstrument-functions to emit EnterExitInstrumenter… (authored by hans). Changed prior to commit: https://reviews.llvm.org/D39331?vs=122889&id=122901#toc Repository: rL LL

[PATCH] D39579: BuiltinOperatorOverloadBuilder: Don't consider types that are unavailable on the target (PR35174)

hans added inline comments. Comment at: lib/Sema/SemaOverload.cpp:7618 - // Define some constants used to index and iterate over the arithemetic types - // provided via the getArithmeticType() method below. - // The "promoted arithmetic types" are the arithmetic + SmallVect

[PATCH] D39579: BuiltinOperatorOverloadBuilder: Don't consider types that are unavailable on the target (PR35174)

This revision was automatically updated to reflect the committed changes. Closed by commit rL318309: BuiltinOperatorOverloadBuilder: Don't consider types that are unavailable on… (authored by hans). Changed prior to commit: https://reviews.llvm.org/D39579?vs=121824&id=123041#toc Repository:

[PATCH] D39994: Loosen MSVC 2017 path requirements

hans added a comment. I think the patch is fine, but Zach should probably sign off on it. https://reviews.llvm.org/D39994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40109: [MS] Apply adjustments after storing 'this'

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D40109 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D40276: Add -finstrument-function-entry-bare flag

hans created this revision. This is an instrumentation flag, similar to -finstrument-functions, but it only inserts calls on function entry, the calls are inserted post-inlining, and they don't take any arugments. This is intended for users who want to instrument function entry with minimal ov

[PATCH] D40276: Add -finstrument-function-entry-bare flag

hans added a comment. Reid, are you happy with this too? In https://reviews.llvm.org/D40276#931502, @pasko wrote: > Instrumenting the function entry post-inlining, without function exit, and > with no parameters is exactly what we need. The > `__cyg_profile_func_enter_bare` sounds good to me a

[PATCH] D40276: Add -finstrument-function-entry-bare flag

This revision was automatically updated to reflect the committed changes. Closed by commit rL318785: Add -finstrument-function-entry-bare flag (authored by hans). Changed prior to commit: https://reviews.llvm.org/D40276?vs=123691&id=123821#toc Repository: rL LLVM https://reviews.llvm.org/D4

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

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

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

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] 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] 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] 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 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] 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] 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 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] D145369: Emit const globals with constexpr destructor as constant LLVM values

hans created this revision. hans added reviewers: rsmith, ilya-biryukov. Herald added a project: All. hans requested review of this revision. Herald added a project: clang. This follows 2b4fa53 which made Clang not emit destructor calls for such objects. However, they would still not get emitted

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

hans added a comment. Interesting! Do you have an example where this (local dllexport/import classes) comes up in practice? MSVC will also allow the following: namespace { struct __declspec(dllexport) T { void foo(); }; } whereas Clang will not, and I think Clang is making the ri

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

hans added a comment. I have no opinion about merging this, but I second the worry about not having a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384 ___

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

hans added a reviewer: mstorsjo. hans added a comment. +mstorsjo for thoughts about Windows code, even if this might not apply to mingw. In D145271#4172636 , @wolfgangp wrote: > A customer complained about the following code (I'm obscuring the class >

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

hans added a reviewer: aaron.ballman. hans added a comment. +aaron.ballman Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145369/new/ https://reviews.llvm.org/D145369 ___ cfe-commits mailing list cfe-comm

[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

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

<    4   5   6   7   8   9   10   11   >