[PATCH] D63648: [Preprocessor] Honor absolute paths in diagnostics

hans added a comment. I will try to take a look, but can you please expand the patch description a little to make it more clear exactly what you're proposing to change? Sorry for all the questions below, I'm just trying to understand exactly what the issue is. > Previously, -fdiagnostics-absol

[PATCH] D64504: Various minor tweaks to CLCompatOptions.td

hans added a comment. Thanks for polishing the UX! :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64504/new/ https://reviews.llvm.org/D64504 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D64253: Let unaliased Args track which Alias they were created from, and use that in Arg::getAsString() for diagnostics

hans added a comment. I think this is going to be my favourite new feature in clang 9 :-) Do you want to put a small note in docs/ReleaseNotes.rst? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64253/new/ https://reviews.llvm.org/D64253 ___

[PATCH] D65302: [clang][docs][release notes] mention asm goto support

hans added a comment. Thanks! Do you want to write something more general for the release notes about the status of Clang and the Linux kernel? Comment at: clang/docs/ReleaseNotes.rst:116 + control flow from inline assembly. The main consumer of this construct is the + Linu

[PATCH] D65302: [clang][docs][release notes] mention asm goto support

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks great! Go ahead and commit directly to the branch (I think it can only be done with SVN still), or let me know if you'd like me to do it. Comment at: clang/docs/ReleaseNo

[PATCH] D65302: [clang][docs][release notes] mention asm goto support

hans added a comment. > I don't have an SVN checkout; would you be so kind as to please merge this on > my behalf? Committed in r367158. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65302/new/ https://reviews.llvm.org/D65302 _

[PATCH] D65302: [clang][docs][release notes] mention asm goto support

This revision was automatically updated to reflect the committed changes. Closed by commit rL367158: [clang][docs][release notes] mention asm goto support (authored by hans, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://r

[PATCH] D65462: gn build: Fix check-clang-tools after r362702.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Seems good to me CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65462/new/ https://reviews.llvm.org/D65462 ___ cfe-commits mailing list cfe-c

[PATCH] D65511: Delay emitting dllexport explicitly defaulted members until the class is fully parsed (PR40006)

hans created this revision. hans added reviewers: rnk, rsmith. This is similar to r245139, but that only addressed dllexported classes. It was still possible to run into the same problem with dllexported members in an otherwise normal class (see bug). This uses the same strategy to fix: delay d

[PATCH] D65511: Delay emitting dllexport explicitly defaulted members until the class is fully parsed (PR40006)

hans marked an inline comment as done. hans added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11545 +for (CXXMethodDecl *M : WorkList) { + DefineImplicitSpecialMember(*this, M, M->getLocation()); + ActOnFinishInlineFunctionDef(M); rn

[PATCH] D65511: Delay emitting dllexport explicitly defaulted members until the class is fully parsed (PR40006)

This revision was automatically updated to reflect the committed changes. hans marked an inline comment as done. Closed by commit rL367520: Delay emitting dllexport explicitly defaulted members until the class is fully… (authored by hans, committed by ). Herald added a project: LLVM. Herald added

[PATCH] D65527: Avoid assemble step in verbose-output-quoting.c

hans added a comment. Sorry, I should have caught this in the original review. Normally tests in Driver/ use the -### flag. That way it doesn't invoke the frontend at all, it just shows how it would invoke it. Would that work for this test also? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D65517: [Preprocessor] Always discard body of #define if we failed to parse it

hans added a comment. In D65517#1609902 , @ilya-biryukov wrote: > @hans, could you please merge rL367530 > into the release? Sure! Let's have it bake in trunk for a bit, and then I'll merge it. Repository: rL LL

[PATCH] D65579: Don't try emitting dllexported explicitly defaulted non-trivial ctors twice during explicit template instantiation definition (PR42857)

hans created this revision. hans added a reviewer: rnk. Trying to emit the definition twice triggers an assert. https://reviews.llvm.org/D65579 Files: clang/lib/Sema/SemaDeclCXX.cpp clang/test/CodeGenCXX/dllexport.cpp Index: clang/test/CodeGenCXX/dllexport.cpp

[PATCH] D65579: Don't try emitting dllexported explicitly defaulted non-trivial ctors twice during explicit template instantiation definition (PR42857)

This revision was automatically updated to reflect the committed changes. Closed by commit rL367661: Don't try emitting dllexported explicitly defaulted non-trivial ctors twice… (authored by hans, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior

[PATCH] D65517: [Preprocessor] Always discard body of #define if we failed to parse it

hans added a comment. In D65517#1609990 , @ilya-biryukov wrote: > SG, just wanted to make sure it's on your list ;-) Merged in r367681. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65517/new/ https://reviews.llvm.org/D

[PATCH] D65655: [clangd] Fix a crash when presenting values for Hover

hans added a comment. In D65655#1612392 , @ilya-biryukov wrote: > @hans, could we merge this commit into the release branch? Sure, merged in r367807. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65655/new/ https://revi

[PATCH] D66031: clangd: use -j for background index pool

hans added a comment. In D66031#1623935 , @sammccall wrote: > @hans If you don't mind merging this, it's a nice usability improvement. Sure! Merged in r368569. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66031/new/ htt

[PATCH] D64672: [X86] Prevent passing vectors of __int128 as in llvm IR

hans added a comment. In D64672#1623784 , @xbolva00 wrote: > Maybe this ABI issue should be fixed for 9.0 ? Not sure how important, but > @hans maybe should track this one. IIUC, this is not a new issue, so I'd prefer not trying to rush it in for llvm

[PATCH] D66074: [clangd] Drop diags from non-written #include.

hans added a comment. In D66074#1624885 , @hokein wrote: > @hans is there still any chance to merge this patch into the release? I tried to merge this to release_90, but I got a test failure. Does this rely on more patches that need merging? Can you try

[PATCH] D66074: [clangd] Drop diags from non-written #include.

hans added a comment. > thanks for the information, I suspect that this patch may depend on rL367303 > (which is not merged in the release), I > will double check. Yes, that seems to be it. I've merged that, and then I could merge this one in r368683. Repo

[PATCH] D66142: clang: Don't warn on unused momit-leaf-frame-pointer when frame pointers are off.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/Driver/frame-pointer-elim.c:1 +// KEEP-ALL-NOT: warning: // KEEP-ALL: "-mframe-pointer=all" This is the first time I've seen t

[PATCH] D66294: [Docs][OpenCL] Release 9.0 notes for OpenCL

hans accepted this revision. hans added a comment. lgtm, and many thanks for writing release notes! Go ahead and commit to the branch directly with SVN or let me know if you'd like me to do it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66294/new/ https://reviews.llvm.org/D66294

[PATCH] D66394: clang-cl: Enable /Zc:twoPhase by default if targeting MSVC 2017 update 3 or newer

hans accepted this revision. hans added a comment. Nice! Please include a mention in docs/ReleaseNotes.rst :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66394/new/ https://reviews.llvm.org/D66394 ___ cfe-commits mailing list cfe-commits

[PATCH] D59415: Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.

hans added inline comments. Comment at: lib/Frontend/TextDiagnostic.cpp:768 if (Dir) { +#ifdef _WIN32 + SmallString<4096> DirName = Dir->getName(); I think this requires a comment explaining why Windows is handled differently. CHANGES SINCE LAST ACTIO

[PATCH] D62270: [Driver] Move the "-o OUT -x TYPE SRC.c" flags to the end of -cc1

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Nice, lgtm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62270/new/ https://reviews.llvm.org/D62270

[PATCH] D59415: Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Thanks! This looks good to me. I'm still a bit nervous about "mklink" in the test, but let's land this and see if the bots can handle it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59

[PATCH] D62276: lld-link, clang: Treat non-existent input files as possible spellos for option flags

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Beautiful! Want to add a line to docs/ReleaseNotes.rst too? :-) Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:177 def warn_drv_unknown_argument_clang_cl_with_s

[PATCH] D62523: Add release note entries for recent typo correction changes

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks great, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62523/new/ https://reviews.llvm.org/D62523 ___ cfe-commits mailing list c

[PATCH] D62490: clang-cl: Fix mangling of catchable types with names longer than 4kiB

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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

hans added a comment. This seems to be stuck. Is there some way we can get incremental progress on this? I'm not very familiar with debug info, but I think it would be a huge win (and somewhat overdue) just to get some very basic tests like rnk's hello.c automated and running continuously. I

[PATCH] D11850: Delay emitting members of dllexport classes until the class is fully parsed (PR23542)

hans added a comment. In D11850#1334344 , @aganea wrote: > Cross-referencing PR40006 . It > seems to be a different manifestation of this same bug. > > @hans Could you possibly take a look at the bug above when

[PATCH] D67304: Emit -Wmicrosoft-enum-value warning instead of error in MS ABI

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

[PATCH] D67606: Change signature of __builtin_rotateright64 back to unsigned

hans added a comment. A simpler way to test this might be to check for conversion warnings in the existing clang/test/CodeGen/avr-builtins.c test, for example like as above. I think that's better since it covers more of the signatures, e.g. the rotateleft ones too. diff --git a/clang/test/Co

[PATCH] D67606: Change signature of __builtin_rotateright64 back to unsigned

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Looks good to me! (with comment) Comment at: test/CodeGen/avr-builtins.c:3 +// Check that the parameter types match. +// RUN: %clang_cc1 -triple avr-unknown-unknown -Wconversio

[PATCH] D68055: Add -fgnuc-version= to control __GNUC__ and other GCC macros

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. This looks reasonable to me. Might be worth mentioning in ReleaseNotes too :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68055/new/ https://revi

[PATCH] D45877: clang-cl: Accept (and ignore) /Zc:__cplusplus.

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

[PATCH] D46406: [docs] Fix typos in the Clang User's Manual.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang https://reviews.llvm.org/D46406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D46520: [Driver] Use -fuse-line-directives by default in MSVC mode

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Seems reasonable to me (but please update the comment before landing). Comment at: lib/Driver/ToolChains/Clang.cpp:4218 // -fno-use-line-directives is default. if (Args.h

[PATCH] D46572: [clang-format] Add raw string formatting to release notes

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. Thanks! Repository: rC Clang https://reviews.llvm.org/D46572 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

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

hans updated this revision to Diff 152659. hans added a comment. Added special-casing for explicit template instantiations, and missing test case suggested by Nico. Please take another look. https://reviews.llvm.org/D48426 Files: include/clang/AST/ExternalASTSource.h include/clang/Basic/L

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

hans added inline comments. Comment at: test/CodeGen/pch-dllexport.cpp:55 +template void __declspec(dllexport) explicitInstantiationDefAfterDecl(T) {} +extern template void explicitInstantiationDefAfterDecl(int); + thakis wrote: > This has two interesting cases

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

This revision was automatically updated to reflect the committed changes. Closed by commit rC335466: [clang-cl] Don't emit dllexport inline functions etc. from pch files (PR37801) (authored by hans, committed by ). Repository: rC Clang https://reviews.llvm.org/D48426 Files: include/clang/AS

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

hans added a comment. This looks reasonable to me as far as I can tell. Thanks! I think the expert on the driver side for this stuff is Nico, so hopefully he can take a look as well. Comment at: include/clang/Basic/DiagnosticLexKinds.td:412 + "%select{create|use}1 precompile

[PATCH] D48601: Added -fcrash-diagnostics-dir flag

hans added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4043-4044 +if (CCGenDiagnostics && A) { + SmallString<128> CrashDirectory; + CrashDirectory = A->getValue(); + llvm::sys::path::append(CrashDirectory, Split.first); zturner wro

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

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. This looks good to me. Comment at: include/clang/Driver/CC1Options.td:604 + HelpText<"When creating a pch stop at this file. When using a pch start " + "after this fi

[PATCH] D48781: [ms] Fix mangling of char16_t and char32_t to be compatible with MSVC.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm with nit Comment at: clang/lib/AST/MicrosoftMangle.cpp:3262 + // Enforce our 32 bytes max, except wchar_t which gets 32 chars instead. + unsigned MaxBytesToWrite = SL->isW

[PATCH] D48601: Added -fcrash-diagnostics-dir flag

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Repository: rL LLVM https://reviews.llvm.org/D48601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D45045: [DebugInfo] Generate debug information for labels.

hans added a comment. In https://reviews.llvm.org/D45045#1150525, @HsiangKai wrote: > In https://reviews.llvm.org/D45045#1114280, @HsiangKai wrote: > > > In https://reviews.llvm.org/D45045#1092697, @hans wrote: > > > > > This broke the Chromium build. I've uploaded a reproducer at > > > https://

[PATCH] D48928: [ms] Fix mangling of string literals used to initialize arrays larger or smaller than the literal

hans created this revision. hans added reviewers: thakis, majnemer. A Chromium developer reported a bug which turned out to be a mangling collision between these two literals: char s[] = "foo"; char t[32] = "foo"; They may look the same, but for the initialization of t we will (under some

[PATCH] D48928: [ms] Fix mangling of string literals used to initialize arrays larger or smaller than the literal

hans added a comment. I couldn't get MSVC to create a symbol with padding at the end; they seem to always insert the padding with a separate loop during initialization, but this does show the truncated case: https://godbolt.org/g/B8ktA3 https://reviews.llvm.org/D48928 __

[PATCH] D48928: [ms] Fix mangling of string literals used to initialize arrays larger or smaller than the literal

hans marked 3 inline comments as done. hans added inline comments. Comment at: lib/AST/MicrosoftMangle.cpp:3198 + ->getSize() + .getZExtValue(); + thakis wrote: > nit: Also do `unsigned StingByteLength = S

[PATCH] D48928: [ms] Fix mangling of string literals used to initialize arrays larger or smaller than the literal

This revision was automatically updated to reflect the committed changes. hans marked 2 inline comments as done. Closed by commit rC336415: [ms] Fix mangling of string literals used to initialize arrays larger or… (authored by hans, committed by ). Changed prior to commit: https://reviews.llvm.

[PATCH] D68554: [clang-format] Proposal for clang-format to give compiler style warnings

hans added a comment. > I can't wait for @hans next Windows Snapshot, this is my gate for rolling > into our builds and CI system, after that, I'm gonna catch every single > person who tries to build/check in unclang-formatted. As it happens, there's a new snapshot out today :-) Repository:

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

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

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

hans added a comment. In D79961#2053806 , @serge-sans-paille wrote: > Bump the version number to be compatible with existing profdata, in a similar > fashion to v1/v2 transition. Did you forget to include some files? I don't see the bump anywhere. =

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

hans added a comment. I worry that the hash version bump isn't complete. Doesn't the hash version used need to be read/written with the profile data file somewhere? Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:148 return PGO_HASH_V1; return PGO_HASH_V2; }

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

hans accepted this revision. hans added a comment. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79961/new/ https://reviews.llvm.org/D79961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/

[PATCH] D79895: Add a new warning to warn when passing uninitialized variables as const reference parameters to a function

hans accepted this revision. hans added a comment. Very nice! I only have minor comments. Also I'm curious to see what this will find in Chromium. I guess we'll find out :-) Comment at: clang/include/clang/Analysis/Analyses/UninitializedValues.h:118 + + virtual void handleCo

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:320 + /// Executable and command-line used to create a given CompilerInvocation. + const char *BuildTool = nullptr; + ArrayRef CommandLineArgs; The name BuildTool makes me think

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:320 + /// Executable and command-line used to create a given CompilerInvocation. + const char *BuildTool = nullptr; + ArrayRef CommandLineArgs; aganea wrote: > hans wrote: > > T

[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#2063979 , @dim wrote: > FWIW, this change causes > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630, see in particular > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c18. > > For some reason, not spawni

[PATCH] D83652: Merge some of the PCH object support with modular codegen

hans added a comment. In D83652#2159585 , @dblaikie wrote: > @hans - could you perhaps give me a quick summary of commands I could use to > test this feature in Chromium (or anything else you might suggest) on a Linux > box? I don't have a Windows machin

[PATCH] D83652: Merge some of the PCH object support with modular codegen

hans added a comment. > Thanks for checking! So, in summary: This seems to work fine for Chromium, > but Chromium isn't really exercising this functionality (only insofar as the > functionality is enabled, but essentially a no-op)? Yup. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D81385: Fix libdl linking for libclang in standalone mode

hans added a comment. In D81385#2162911 , @mstorsjo wrote: > Ping @beanz > > @hans - I think this is something that would be wanted to have fixed in the > release branch (it's a regression for certain build configurations, afaik) - > the fix awaits an ac

[PATCH] D83592: [Coverage] Add comment to skipped regions

hans added a comment. This caused asserts in Chromium's coverage builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1108352 I've reverted it in the meantime (238bbd48c5a5f84deca36e5df980241578f7c1df ). It also looks

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

hans added inline comments. Comment at: clang/unittests/CMakeLists.txt:14 +if (CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG) + add_definitions("-Wno-suggest-override") +endif() Because it's added as a define, this gets passed to rc.exe in Windows builds, which is used fo

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

hans added a comment. In D84244#2167602 , @logan-5 wrote: > Thanks for reverting--I agree that that's the right move at this point. > > Pretty much totally out my depth here, and I don't have a way of debugging > the Windows issue, so I'm not sure how to

[PATCH] D84244: [clang] Disable -Wsuggest-override for unittests/

hans added a comment. > In that case, I've enabled it again using `add_compile_options` instead of > `add_definitions`. I've got my finger hovering over the button to revert. (For reference, the re-commit is 77e0e9e .) Looks

[PATCH] D83592: [Coverage] Add comment to skipped regions

hans added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:973 + if ((LexLevel == 0 || PreprocessToken) && + !Result.getFlag(Token::IsReinjected)) { +if (LexLevel == 0) zequanwu wrote: > @hans , can you take a look on this part? I saw `Token

[PATCH] D81385: Fix libdl linking for libclang in standalone mode

hans added a comment. Pushed to 11.x as 833f8c958601bb640ba6a25d627c1dc58dad14d2 . Please let me know if there are any follow-ups. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D8

[PATCH] D83592: [Coverage] Add comment to skipped regions

hans added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:973 + if ((LexLevel == 0 || PreprocessToken) && + !Result.getFlag(Token::IsReinjected)) { +if (LexLevel == 0) zequanwu wrote: > hans wrote: > > zequanwu wrote: > > > @hans , can you

[PATCH] D83817: [clangd] Add option to use remote index as static index

hans added inline comments. Comment at: clang-tools-extra/clangd/Features.inc.in:2 #define CLANGD_BUILD_XPC @CLANGD_BUILD_XPC@ +#define CLANGD_ENABLE_REMOTE @CLANGD_ENABLE_REMTE@ I'm guessing this should be @CLANGD_ENABLE_REMOTE@ The gn build was upset: http://4

[PATCH] D83817: [clangd] Add option to use remote index as static index

hans added inline comments. Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:702 } + if (RemoteIndexAddress.empty() != ProjectPath.empty()) { +llvm::errs() << "remote-index-address and project-path have to be " hokein wrote: > the new code section

[PATCH] D77585: Stop passing site cfg files via --param to llvm-lit.

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/CMakeLists.txt:113 -set(ANALYZER_TEST_PARAMS - USE_Z3_SOLVER=0) This and ANALYZER_TEST_PARAMS_Z3 are just dropped because they're

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added a comment. In D80833#2071818 , @aganea wrote: > I didn't change the PWD/CWD stored in `LF_BUILDINFO`, it has already been > done by Reid a while ago: D53179 (it > already stores absolute paths) That's surpri

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans added a comment. > The only absolute paths that remain are: a. the compiler path > (`D:\llvm-project\buildninjaDebMSVC\bin\clang-cl.exe` in the yaml below) and > b. the `-internal-isystem` paths. However that is not an issue on our end, as > we're building with `-nostdinc` + nuget packages

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/CodeGen/debug-info-codeview-buildinfo.c:3 +// RUN: llvm-pdbutil dump --types %t.obj | FileCheck %s +// RUN: %clang_cl /c /Z7 %s /Fo%t.obj -fdebug-comp

[PATCH] D80153: [AST] Mangle LambdaContextDecl for top level decl

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

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added a comment. In D83013#2132070 , @echristo wrote: > Adding Chandler and Alina here as well. > > In general, I don't think that this is such a great idea. Being able to have > this sort of thing work more reliably is one of the reasons for the new

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added a comment. > I don't want to block this patch, but I do agree with Eric's point. We > *really* want to focus more on the switch then invest into more LPM infra. > Short term resolutions to unblock folks, with the best split possible, sure, > keeping in mind they'll need to be cleaned

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

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

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added inline comments. Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:170 PassManagerBuilder::PassManagerBuilder() { OptLevel = 2; Oh, just noticed: I think CallGraphProfile should be initialized along with the other flags here. ==

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added a comment. In D83013#2137607 , @MaskRay wrote: > `Opts.getProfileUse() != CodeGenOptions::ProfileNone ` in > > Opts.CallGraphProfile = Opts.getProfileUse() != CodeGenOptions::ProfileNone > && > !Opts.DisableIntegra

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans accepted this revision. hans added a comment. In D83013#2142271 , @nikic wrote: > New compile-time numbers: > https://llvm-compile-time-tracker.com/compare.php?from=0b39d2d75275b80994dac06b7ad05031cbd09393&to=fd070b79e063fff2fad3cd4a467f64dfca83eb90&

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans accepted this revision. hans added a comment. Still lgtm. For what it's worth, I think you could have just re-committed with the fixes rather than uploading for review again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://re

[PATCH] D83652: Merge some of the PCH object support with modular codegen

hans added a comment. In D83652#2146732 , @llunak wrote: > > Do either of you know if it'd be practical to move to something more > > similar to .pcm handling, where the pch itself is passed to the > > compilation, rather than homed as a side effect of c

[PATCH] D83592: [Parser] Add comment to skipped regions

hans added a comment. > Not sure if this is good idea to untrack comments, it breaks many tests under > CoverageMapping. I didn't look, but I'm surprised there are many coverage tests that have comments in them. If the comments are not important for those tests, maybe they could be removed? As

[PATCH] D83592: [Parser] Add comment to skipped regions

hans added a comment. > The comments in those coverage tests are used for FileCheck, like `//CHECK:`. > So, we can't remove those ones. Oh, I didn't think about that :-) It's a bit unusual and annoying that the test expectations end up affecting the test output. For the comments that are not r

[PATCH] D79895: Add a new warning to warn when passing uninitialized variables as const reference parameters to a function

hans added a comment. In D79895#2107604 , @nick wrote: > This diagnostic bring headaches because frequently `-Wunused-variable` > suppression is done via no-op pseudo-consuming function like > `boost::ignore_unused` >

[PATCH] D79895: Add a new warning to warn when passing uninitialized variables as const reference parameters to a function

hans added a comment. In D79895#2109414 , @nick wrote: > > I feel like doing interprocedural analysis for this is overkill. What is > > the benefit of boost::ignore_unused(foo); rather than the more common > > (void) foo;? Any examples? > > > > > I haven

[PATCH] D82425: [SemaCXX] Fix false positive of -Wuninitialized-const-reference in empty function body.

hans added a comment. Okay, since checking this is cheap I suppose we can do it. Comment at: clang/lib/Analysis/UninitializedValues.cpp:410 + if (FunctionDecl *fd = CE->getDirectCallee()) { +if (FunctionTemplateDecl *ftd = fd->getPrimaryTemplate()) + return ftd->getTe

[PATCH] D53891: [LTO] Add option to enable LTOUnit splitting, and disable unless needed

hans added a comment. Herald added a subscriber: hiraditya. Would be possible to add some documentation for this flag? From a quick search it's not clear to me what it does and when one is supposed to use it. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53891/new

[PATCH] D82862: [ThinLTO] Always parse module level inline asm with At&t dialect

hans created this revision. hans added a reviewer: pcc. Herald added subscribers: llvm-commits, dexonsmith, steven_wu, hiraditya, inglorion. Herald added a project: LLVM. clang-cl passes -x86-asm-syntax=intel to the cc1 invocation so that assembly listings produced by the /FA flag are printed in

[PATCH] D82862: [ThinLTO] Always parse module level inline asm with At&t dialect

This revision was automatically updated to reflect the committed changes. Closed by commit rGa8e582c8307b: [ThinLTO] Always parse module level inline asm with At&t dialect (PR46503) (authored by hans). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D65543: [Windows] Autolink with basenames and add libdir to libpath

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. I agree with the sentiment that it's annoying when clang doesn't just work out of the box. But it will still do, except for asan and profile info etc, for which I think it's reasonable to add to t

[PATCH] D65543: [Windows] Autolink with basenames and add libdir to libpath

hans added a comment. The docs look great, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65543/new/ https://reviews.llvm.org/D65543 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D78879: [clang-format] [PR45357] Fix issue found with operator spacing

hans added a subscriber: tstellar. hans added a comment. In D78879#2004751 , @MyDeveloperDay wrote: > So for the Mozilla case mentioned in the bug report we either need to ask > @hans if D76850: clang-format: Fix pointer alignment for overloaded > oper

[PATCH] D78534: [libclang] Install both libclang.a and libclang.so when LIBCLANG_BUILD_STATIC=ON

hans added a comment. This broke builds configured with -DLLVM_ENABLE_PIC=OFF, e.g. $ cmake -GNinja -DCMAKE_BUILD_TYPE=Release '-DLLVM_ENABLE_PROJECTS=clang' '-DLLVM_TARGETS_TO_BUILD=X86' -DLLVM_ENABLE_PIC=OFF ../llvm

[PATCH] D78429: [clangd] Metric tracking through Tracer

hans added a comment. For anyone running into the same problem, this broke the build with GCC 5: /work/llvm.monorepo/clang-tools-extra/clangd/ClangdServer.cpp:374:75: error: could not convert ‘(const char*)""’ from ‘const char*’ to ‘llvm::StringLiteral’

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