[PATCH] D79219: [CMake] Simplify CMake handling for zlib

hans added a comment. In D79219#2153585 , @phosek wrote: > In D79219#2152747 , @labath wrote: > >> I wouldn't mind separate (internal) cache variable, though I am somewhat >> surprised by this problem. A (non-cache

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

hans added a comment. Reverted in 3ab01550b632dad46f9595d74855749557ffd25c Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79219/new/ https://reviews.llvm.org/D79219

[PATCH] D84782: [PGO] Include the mem ops into the function hash.

hans added a comment. In D84782#2191429 , @yamauchi wrote: > In D84782#2191243 , @MaskRay wrote: > >> @yamauchi Do we need cd890944ad344b1b8cac58332ab11c9eec6b61e9 >>

[PATCH] D85252: [Driver] Accept -fno-lto in clang-cl

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm In general when -ffoo is supported by clang-cl, -fno-foo should be too, so this makes sense. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D852

[PATCH] D85390: [Clang] Add note for bad conversion error when expression is of forward-declared type

hans added a comment. Thanks! This looks promising. In addition to updating existing tests, it would be good to add a test aimed explicitly at this. Maybe you can reduce something from the example in the bug report. Maybe looking at git blame for emitBadConversionNotes() will provide hints for

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

hans added inline comments. Comment at: llvm/cmake/config-ix.cmake:178 -if (LLVM_ENABLE_ZLIB STREQUAL "FORCE_ON" AND NOT HAVE_LIBZ) - message(FATAL_ERROR "Failed to configure zlib") Could this check be put back? E.g. for now it seems building with -DLLVM_USE_

[PATCH] D85390: [Clang] Add note for bad conversion error when expression is of forward-declared type

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks great, thanks! I put a suggestion for different wording of the note, but I'll leave it up to you to decide. Comment at: clang/test/SemaCXX/pointer-forward-declared-class-c

[PATCH] D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer

hans added a comment. I suppose this is just piling on heuristics, but I guess it fixes the reported bug. Krasimir, what do you think? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79293/new/ https://reviews.llvm.org/D79293 ___ cfe-commits m

[PATCH] D79417: Don't assert about missing profile info in createProfileWeightsForLoop

hans created this revision. hans added reviewers: thakis, rnk. The compiler shouldn't crash if the profile info is slightly off. We hit this in Chromium. https://reviews.llvm.org/D79417 Files: clang/lib/CodeGen/CodeGenPGO.cpp Index: clang/lib/CodeGen/CodeGenPGO.cpp

[PATCH] D79417: Don't assert about missing profile info in createProfileWeightsForLoop

hans added a comment. In D79417#2020607 , @thakis wrote: > Could there be a test for this? Or is that difficult? I think it would be a bit of a pain, which is why I skipped it. It also wouldn't be a very interesting test, just "don't assert here", reall

[PATCH] D79417: Don't assert about missing profile info in createProfileWeightsForLoop

This revision was automatically updated to reflect the committed changes. Closed by commit rG55b9b11fea3e: Don't assert about missing profile info in createProfileWeightsForLoop (authored by hans). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D79210: Let clang print registered targets for --version

hans added a comment. I think this is worth a short note in docs/ReleaseNotes.rst. Also this broke Goma which we use in Chromium (https://chromium.googlesource.com/infra/goma/client/) because it's a bit too strict when parsing the output of "clang -v" -- but that's our problem and we'll fix. :

[PATCH] D79531: Make -Wnonportable-include-path ignore drive case on Windows.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks reasonable to me, but Windows paths are scary.. Comment at: clang/lib/Lex/PPDirectives.cpp:2112 +// letter for absolute paths, but Name might start with either +//

[PATCH] D79599: Add a flag that controls if clang-tidy and clang-include-fixer are built into libclang.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks very reasonable to be. Maybe worth a release note in case this is surprising to any libclang users? And maybe cc the other folks from D55415 ? ===

[PATCH] D79595: Fix bugs when an included file name is typo corrected.

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

[PATCH] D79916: Map -O to -O1 instead of -O2

hans added a reviewer: echristo. hans added a comment. Actually it was http://llvm.org/r82131 that mapped -O to -O2 (I just refactored it). Originally it seems it was mapped to -O1. Because this seems to have done quite intentionally, I'm a little vary of changing it back. I'm not sure who woul

[PATCH] D80014: Turn -Wmax-tokens off by default

hans created this revision. hans added a reviewer: thakis. Herald added a project: clang. On the one hand, one might interpret the use of the max-token pragmas or -fmax-tokens flag as an opt-in to the warning. However, in Chromium we've found it useful to only opt in selected build configuration

[PATCH] D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

hans added a comment. > clang issues a warning, leaving no good way to print SIZE_T (other than > disabling warnings or adding useless casts) I also don't think Clang should change here though. The warning is legit for code that cares about portability, and inserting a cast is probably the best

[PATCH] D79961: [PGO] Fix computation of fuction Hash

hans added a comment. Nice fix, and nice test :-) May I ask how you found this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79961/new/ https://reviews.llvm.org/D79961 ___ cfe-commits mailing list cf

[PATCH] D80014: Turn -Wmax-tokens off by default

This revision was automatically updated to reflect the committed changes. Closed by commit rG87b235db63a3: Turn -Wmax-tokens off by default (authored by hans). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80014/new/ https://reviews.llvm.org/D80014

[PATCH] D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

hans added a comment. Doing special handling for SIZE_T, as opposed to long in general, on Windows as Richard suggests seems reasonable to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78508/new/ https://reviews.llvm.org/D78508

[PATCH] D40621: MS ABI: Treat explicit instantiation definitions of dllimport function templates as explicit instantiation decls (PR35435)

hans created this revision. This matches MSVC's behaviour, and we already do it for class templates since r270897. https://reviews.llvm.org/D40621 Files: lib/Sema/SemaTemplate.cpp test/CodeGenCXX/dllimport.cpp Index: test/CodeGenCXX/dllimport.cpp =

[PATCH] D40621: MS ABI: Treat explicit instantiation definitions of dllimport function templates as explicit instantiation decls (PR35435)

This revision was automatically updated to reflect the committed changes. Closed by commit rL319386: MS ABI: Treat explicit instantiation definitions of dllimport function… (authored by hans). Changed prior to commit: https://reviews.llvm.org/D40621?vs=124808&id=124839#toc Repository: rL LLV

[PATCH] D40746: Correctly handle line directives without filenames that come first in the file

hans created this revision. The comment in LineTableInfo::AddLineNote says "An unspecified FilenameID means use the last filename if available, or the main source file otherwise.", but the second part of that sentence was never actually implemented. This lead to asserts when writing the line table

[PATCH] D40746: Correctly handle line directives without filenames that come first in the file

hans added a comment. In https://reviews.llvm.org/D40746#942824, @rsmith wrote: > The intent is to use a `FilenameID` of -1 to represent this situation; see > the documentation of the `LineEntry::FilenameID` member. Users of that field > are expected to deal with that value (see the handling of

[PATCH] D40746: Correctly handle line table entries without filenames during AST serialization

hans updated this revision to Diff 125227. hans retitled this revision from "Correctly handle line directives without filenames that come first in the file" to "Correctly handle line table entries without filenames during AST serialization". hans edited the summary of this revision. https://revi

[PATCH] D40746: Correctly handle line table entries without filenames during AST serialization

This revision was automatically updated to reflect the committed changes. Closed by commit rC319707: Correctly handle line table entries without filenames during AST serialization (authored by hans). Repository: rC Clang https://reviews.llvm.org/D40746 Files: lib/Serialization/ASTReader.cpp

[PATCH] D40746: Correctly handle line table entries without filenames during AST serialization

This revision was automatically updated to reflect the committed changes. Closed by commit rL319707: Correctly handle line table entries without filenames during AST serialization (authored by hans). Changed prior to commit: https://reviews.llvm.org/D40746?vs=125227&id=125419#toc Repository:

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

hans added a comment. For more context, this was initially raised in https://discourse.llvm.org/t/why-is-the-deprecated-copy-warning-suppressed-in-msvc-compatibility-mode/65085/1 I'm not aware of any reason for the special microsoft mode case, but it's very possible that I'm missing something.

[PATCH] D133457: Add Clang driver flags equivalent to cl's /MD, /MT, /MDd, /MTd.

hans added inline comments. Comment at: clang/include/clang/Driver/Options.td:2209 +def fms_runtime_lib_EQ : Joined<["-"], "fms-runtime-lib=">, Group, + Flags<[NoXarchOption, CoreOption]>, HelpText<"Select Windows run-time library">; defm delayed_template_parsing : BoolFOption

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

hans added inline comments. Comment at: clang/test/SemaCXX/deprecated-copy-msvc.cpp:1 +// RUN: %clang_cc1 %s -std=c++11 %s -Wdeprecated-copy -verify -fms-compatibility + Can we just add a RUN line with -fms-compatibility to clang/test/SemaCXX/deprecated-copy.cp

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Maybe give Richard another day or two in case he wants to take a look, but otherwise landing this and seeing if there are any problems we didn't know about sounds good to me. CHANGES SINCE

[PATCH] D133457: Add Clang driver flags equivalent to cl's /MD, /MT, /MDd, /MTd.

hans added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4442 +bool IsClangCL) { + unsigned RTOptionID = 0; // MT=0, MTd=1, MD=2, MDd=3 + bool HasLDdFlag = IsClangCL && Args.hasArg(options::OPT__SLASH_LDd); --

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

This revision was automatically updated to reflect the committed changes. Closed by commit rG49e7ef2c09fa: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode (authored by ningvin, committed by hans). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D133354: [Clang]: Diagnose deprecated copy operations also in MSVC compatibility mode

hans added a comment. In D133354#3789212 , @ningvin wrote: > @hans just to clarify as I do not have commit access: do I still need to > perform some action? Ah, thanks for letting me know. I've committed on your behalf as https://github.com/llvm/llvm-p

[PATCH] D133457: Add Clang driver flags equivalent to cl's /MD, /MT, /MDd, /MTd.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm, nice! Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4452 + + if (Arg *A = Args.getLastArg(options::OPT_fms_runtime_lib_EQ)) { +StringRef Val = A->getValue(); -

[PATCH] D133920: [X86][fastcall] Move capability check before free register update

hans added a comment. Instead of just referring to the bug in the commit message, can you add some text explaining the effects of this change, e.g. on the `__fastcall void f(unsigned long long a, int b, int c)` function in the bug and that this matches MSVC? Did you double check whether this m

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

hans added a comment. Could the new flag be marked as an Alias in the tablegen, so that we don't need to do any code changes for it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133959/new/ https://reviews.llvm.org/D133959 __

[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] 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. 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. > 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] D128409: [clang-cl] Add -emit-ast to clang-cl driver

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. It feels kind of weird to use the `/Fo` option for this that's for object files, but if that makes using the static analyzer more convenient that's okay I suppose. Comment at:

[PATCH] D128649: [clang-cl] Accept a pragma alloc_text corner case accepted by MSVC

hans added inline comments. Comment at: clang/lib/Sema/SemaAttr.cpp:811 return; } Since the pragma only applies to functions, maybe we should error here if the decl is not a FunctionDecl? Comment at: clang/lib/Sema/SemaAttr.cpp:8

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

hans added a comment. > Regarding /Fo bit - if you want I can change it to only handle /o in this if > statement. I don't mind either way. Just doing `/o` seems better. Thanks. Comment at: clang/test/Driver/ast.c:30 +// Also check clang-cl since the driver is slightly differe

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

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

[PATCH] D128649: [clang-cl] Handle some pragma alloc_text corner cases handled by MSVC

hans added a comment. In D128649#3612626 , @steplong wrote: > I think we have to use `isInExternCContext()` to accept the following (MSVC > accepts this): > > extern "C" { static void f(); } > static void f() {} I'm probably missing something. Is th

[PATCH] D128649: [clang-cl] Handle some pragma alloc_text corner cases handled by MSVC

hans added a comment. In D128649#3612875 , @steplong wrote: >> Isn't the question whether `f` is considered "extern C" in the end or not? I >> thought `isExternC()` checks that? Are you saying it would return false for >> `f` in your example? > > Yup, `

[PATCH] D128649: [clang-cl] Handle some pragma alloc_text corner cases handled by MSVC

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

[PATCH] D122547: [clang] Make Driver tests pass when running with temp dir containing "crt"

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

[PATCH] D122766: [clang] Use forward slash as the path separator for Windows in __FILE__, __builtin_FILE(), and std::source_location

hans added a comment. > we should have a flag that controls which slash direction to use on windows > triples, since different people will want different things. > And since most people who use clang-cl run it on Windows, the default for > that flag should imho stay a backslash (but projects can

[PATCH] D122766: [clang] Use forward slash as the path separator for Windows in __FILE__, __builtin_FILE(), and std::source_location

hans added reviewers: mstorsjo, aaron.ballman. hans added a comment. Maybe Martin or Aaron have opinions here too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122766/new/ https://reviews.llvm.org/D122766 _

[PATCH] D122766: [clang] Use forward slash as the path separator for Windows in __FILE__, __builtin_FILE(), and std::source_location

hans added a comment. In D122766#3420925 , @thakis wrote: > Do you know what cl.exe does to paths in pdb files? Does it write > mixed-slashiness for foo/bar.h includes, or does it write backslashes > throughout? Looks like just backslashes: cl /nolo

[PATCH] D120374: [clang-format] Do not insert space after new/delete keywords in C function declarations

hans added a comment. Herald added a project: All. This seems to have caused https://github.com/llvm/llvm-project/issues/54703 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120374/new/ https://reviews.llvm.org/D120374 _

[PATCH] D123070: [Driver][NFC] Simplify handling of flags in Options.td

hans accepted this revision. hans added a comment. Looks reasonable to me. I worry that the closing comments on '}'s might rot since there's no tooling to maintain them though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123070/new/ https://rev

[PATCH] D122766: [clang] Use forward slash as the path separator for Windows in __FILE__, __builtin_FILE(), and std::source_location

hans added a comment. > My feeling is that the default behavior on Windows needs to be to use > backslashes and not forward slashes. Okay, how would folks feel about always canonicalizing `__FILE__` etc. to use //backslashes// when targeting Windows? Repository: rG LLVM Github Monorepo CHA

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

hans added a comment. Here's an example where I think this regressed a Clang diagnostic. Consider: template struct Template { Template(int x) {} }; struct S1 { struct Foo; typedef Template Typedef; }; struct S2 { struct Foo; typedef Template Typedef; }; typed

[PATCH] D106790: prfchwintrin.h: Make _m_prefetchw take a pointer to volatile (PR49124)

hans created this revision. hans added reviewers: thakis, rnk. hans requested review of this revision. Herald added a project: clang. For some reason, Microsoft declares _m_prefetch to take a const void*, but _m_prefetchw to take a /volatile/ const void*. I can't think of any downside to just ca

[PATCH] D106791: [clang-cl] Expose -fmodules and related flags in the driver (PR43391)

hans created this revision. hans added reviewers: thakis, rnk. Herald added a subscriber: dang. hans requested review of this revision. Herald added a project: clang. I don't know how well this works with clang-cl, but people want to try it out, and I think we want to make it work, so exposing th

[PATCH] D106790: prfchwintrin.h: Make _m_prefetchw take a pointer to volatile (PR49124)

hans added a comment. In D106790#2905110 , @rnk wrote: > Does this match GCC? They also provide this Intel intrinsic. No, they define it without volatile (and also without const): https://github.com/gcc-mirror/gcc/blob/releases/gcc-11.1.0/gcc/config/i38

[PATCH] D106791: [clang-cl] Expose -fmodules and related flags in the driver (PR43391)

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa648f3434274: [clang-cl] Expose -fmodules and related flags in the driver (PR43391) (authored by hans). Repository: rG LLVM Github Monorepo CHANG

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

hans added a comment. This is causing the compiler to crash in Chromium builds, see https://bugs.chromium.org/p/chromium/issues/detail?id=1195353 I verified that replacing the use of getLiteralData() with OriginalFileName in HandleHeaderIncludeOrImport() fixes the crash, but the the other getL

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

hans added a comment. In D106394#2907520 , @hans wrote: > This is causing the compiler to crash in Chromium builds, see > https://bugs.chromium.org/p/chromium/issues/detail?id=1195353 Durr, that should be https://bugs.chromium.org/p/chromium/issues/det

[PATCH] D106898: Revert "Revert "[clang][pp] adds '#pragma include_instead'"" Includes regression test for problem noted by @hans.

hans added inline comments. Comment at: clang/test/PCH/ms-pch-macro-include_instead-regression.c:24 + +#include BAZ + This is really the interesting part. Maybe the other checks and macros could be removed, if that's enough to trigger the crash? Repository:

[PATCH] D106790: prfchwintrin.h: Make _m_prefetchw take a pointer to volatile (PR49124)

hans added a comment. > Sure, they won't be included together, but will GCC users get warnings from > passing non-volatile pointers to `_mm_prefetchw`? If we change these > qualifiers to match MSVC, will GCC/Clang users be able to observe any change > in behavior? I don't think that should cau

[PATCH] D122298: [clang-cl] Ignore /Wv and /Wv:17 flags

hans accepted this revision. hans added a comment. Looks great, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122298/new/ https://reviews.llvm.org/D122298 ___ cfe-commits mailing list cfe-commits

[PATCH] D123070: [Driver][NFC] Simplify handling of flags in Options.td

hans added a comment. In D123070#3430207 , @ekieri wrote: > @hans , thanks, that is a valid concern. I was asked to add closing comments > on an earlier patch though, and I find them particularly useful (when > accurate...) in this file, which does not

[PATCH] D123405: [dllexport] odr-use constexpr default args for constructor closures

hans created this revision. hans added reviewers: rnk, mstorsjo, thakis. Herald added a project: All. hans requested review of this revision. Herald added a project: clang. InstantiateDefaultCtorDefaultArgs() is supposed to mark default constructor args as odr-used, since those args will be used

[PATCH] D123405: [dllexport] odr-use constexpr default args for constructor closures

hans added a comment. I think @thakis's review is sufficient. I am of course happy to follow up on any extra review with follow-up patches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123405/new/ https://reviews.llvm.org/D123405 ___

[PATCH] D123405: [dllexport] odr-use constexpr default args for constructor closures

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG40ad6670138a: [dllexport] odr-use constexpr default args for constructor closures (authored by hans). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D122766: [clang] Use forward slash as the path separator for Windows in __FILE__, __builtin_FILE(), and std::source_location

hans added a comment. In D122766#3450298 , @ayzhao wrote: > So, the general consensus seems to be that we should use backslashes when > targeting Windows. > > I implemented using only backslashes for Windows; however, > clang/test/SemaCXX/destructor.cpp

[PATCH] D123957: Update the developer policy to mention release notes

hans added a comment. If it's possible, maybe this could be worded in a way that strongly suggests the release notes should ideally be updated in the same patch as the code change? I think we're generally good at doing this for tests. It would be great if we could treat release notes the same.

[PATCH] D123957: Update the developer policy to mention release notes

hans added a comment. Looks great to me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123957/new/ https://reviews.llvm.org/D123957 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co

[PATCH] D124057: [asan] Enable detect_stack_use_after_return=1 by default

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Sounds good to me. Worth a release note maybe? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124057/new/ https://reviews.llvm.org/D124057

[PATCH] D118762: Remove -Wweak-template-vtables

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG564f9be11c9c: Remove -Wweak-template-vtables (authored by hans). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added a comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/ma

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added a comment. It seems Phabricator ate my comment, but I meant to say: My understanding is still that IR passes generally live in Transforms/ and that CodeGen/ deals with lower levels such as MachineInstrs, SelectionDAG, etc. with CodegenPrepare as the big exception. Thinking about it

[PATCH] D118980: Don't dllexport reference temporaries

hans created this revision. hans added reviewers: thakis, mstorsjo, rnk. hans requested review of this revision. Herald added a project: clang. Even if the reference itself is dllexport, the temporary should not be -- in fact, we're already giving it internal linkage, so dllexporting it is not ju

[PATCH] D118980: Don't dllexport reference temporaries

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG853e0aa424e4: Don't dllexport reference temporaries (authored by hans). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D117500: [clang] Mention MS on-demand TLS initialization in release notes

hans closed this revision. hans added a comment. Oh, right. And since the change landed before the branch, this should go on the 14.x release branch. Landed here: https://github.com/llvm/llvm-project/commit/20ea9e379984f240d7135843baae6999abf3bf2b Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added inline comments. Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:180 + "JMC instrument more than once?"); + FunctionCallee Fn = + M.getOrInsertFunction(CheckFunctionName, getCheckFunctionType(Ctx)); might as well get the Func

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added inline comments. Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1096 + if (TM->Options.JMCInstrument) +addPass(createJMCInstrumenterPass()); addCodeGenPrepare(); ychen wrote: > hans wrote: > > could this be moved into addIRPasses()? > Yes, i

[PATCH] D118428: [clang-cl] Support the /JMC flag

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

[PATCH] D119446: [clang-cl] Accept the "legacy'"-target flag spelling

hans created this revision. hans added a reviewer: thakis. Herald added a subscriber: dang. hans requested review of this revision. Herald added a project: clang. we already accept "--target=". No reason to not accept "-target" too (that's the one I typically use for some reason). Repository:

[PATCH] D119446: [clang-cl] Accept the "legacy'"-target flag spelling

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8baa076dffa3: [clang-cl] Accept the "legacy" -target flag spelling (authored by hans). Changed prior to commit: https://reviews.llvm.org/D119446?v

[PATCH] D119574: [clang] Expose -fprofile-use in clang-cl

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

[PATCH] D109611: Fix CLANG_ENABLE_STATIC_ANALYZER=OFF building all analyzer source

hans added a comment. I tried cmake -GNinja -DCLANG_ENABLE_STATIC_ANALYZER=OFF -DLLVM_ENABLE_PROJECTS="llvm;clang;clang-tools-extra" -DCLANG_ENABLE_ARCMT=OFF ../llvm and verified that the breakage occurs at 6d7b3d6b3a8dbd62650b6c3dae1fe904a8ae9048

[PATCH] D119574: [clang] Expose -fprofile-use in clang-cl

hans added a comment. In D119574#3316216 , @paulkirth wrote: > Hi, > > I think we hit a test failure due to this patch in Fuchsia's Clang canary for > Windows. We've had a lot of breakages on Windows today, so this was hidden > until 0574b5fc >

[PATCH] D78762: build: use `find_package(Python3)` if available

hans added a comment. Herald added a project: clang-tools-extra. What's the new cmake flag to set the python executable, and should we update llvm/docs/GettingStarted.rst to not refer to PYTHON_EXECUTABLE anymore? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D124057: [asan] Enable detect_stack_use_after_return=1 by default

hans accepted this revision. hans added inline comments. Comment at: clang/docs/ReleaseNotes.rst:175 - Remove anonymous tag locations. - Beautify dump format, add indent for nested struct and struct members. +- Previously disabled sanitizer options now enabled by default --

[PATCH] D116527: [clang-format] Fix indentation for array variables with alignment of consecutive assignments and declarations.

hans added a comment. Herald added a project: All. This seems to have caused a strange indentation problem, see https://github.com/llvm/llvm-project/issues/55161 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116527/new/ https://reviews.llvm.org/D1

[PATCH] D116527: [clang-format] Fix indentation for array variables with alignment of consecutive assignments and declarations.

hans added a comment. In D116527#3479509 , @curdeius wrote: > Feel free to revert and add a regression test please. I'll take a look. Since this patch landed a couple of months back, I don't think we need to rush to revert it. Repository: rG LLVM Gi

[PATCH] D124701: [clang] Honor __attribute__((no_builtin("foo"))) on functions

hans added a comment. If I understand correctly, D68028 made it so that LLVM doesn't add any builtin calls (e.g. turning for-loops into memcpy), but Clang could still turn calls into builtins. Maybe the patch description could be expanded to explain this? ==

[PATCH] D124702: [MSVC] Add support for pragma function

hans added a comment. From the MS docs: > Once a function pragma is seen, it takes effect at the first function > definition that contains a specified intrinsic function. The effect continues > to the end of the source file, or to the appearance of an intrinsic pragma > specifying the same int

[PATCH] D124702: [MSVC] Add support for pragma function

hans added a comment. >> And should we error/warn if the pragma occurs not in namespace scope? > > Oh I wasn't sure how to check if the function pragma was outside of a > function. In the ActOn.. methods I think you can check what the CurContext is. Comment at: clang/lib/Pa

[PATCH] D124702: [MSVC] Add support for pragma function

hans added a comment. It needs tests for the warnings about badly formed pragmas, and for pragmas outside file/namespace scope. Comment at: clang/lib/Parse/ParsePragma.cpp:3561 << "intrinsic"; return; } steplong wrote: > hans wrote: > > since

[PATCH] D129226: [clang/mac] Make -mmacos-version-min the canonical spelling over -mmacosx-version-min

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm, but maybe wait a few more days in case the apple folks want to chime in CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129226/new/ https://reviews.llvm.org/D129226 _

[PATCH] D128927: [libc++] Always build c++experimental.a

hans added a comment. In D128927#3648652 , @ldionne wrote: > In D128927#3648616 , @fhahn wrote: > >> It looks like this is causing macOS builds to fail on GreenDragon. Please >> take a look and refer the commit i

[PATCH] D128927: [libc++] Always build c++experimental.a

hans added a comment. After some more digging, it seems we're hitting the same issue as we did back in https://reviews.llvm.org/D82702#2153130 and now the work-around of passing `-DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF` stopped working. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D128927: [libc++] Always build c++experimental.a

hans added a subscriber: thakis. hans added a comment. After discussing with @thakis a bit, I've reverted this in 09cebfb978def7fa2a4460bca89690f8d3608216 to bring builds back to a green state while we figure out what's going

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