[PATCH] D70701: Fix more VFS tests on Windows

rnk accepted this revision. rnk added a subscriber: JDevlieghere. rnk added a comment. This revision is now accepted and ready to land. +@JDevlieghere, due to recent VFS test fixup. I think this looks good, and in the absence of input from other VFS stakeholders, let's land this soon. Thanks!

[PATCH] D71572: [ItaniumCXXABI] Make tls wrappers comdat on Windows

rnk added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2551 + // actually handle multiple TUs defining the same wrapper. + if (CGM.getTriple().isOSWindows() && CGM.supportsCOMDAT() && + Wrapper->isWeakForLinker()) IMO we should be doing

[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

rnk added a comment. Thanks! I made some comments. This code is convoluted. I don't think I can reason through all the edge cases, but this seems like an improvement. I'd suggest adding the recommended tests and any other corner case inputs you can think of, and after another round of review we

[PATCH] D71576: [c++20] Add deprecation warnings for the expression forms deprecated by P1120R0.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I have no blocking concerns, just some idle thoughts. Up to you if you want Aaron's feedback before landing. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6235 + "

[PATCH] D71039: Add support for the MS qualifiers __ptr32, __ptr64, __sptr, __uptr.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks great to me. This has the potential to break some existing code, though. I would suggest either landing it early in the day, watching for breakage, and hoping for the best, or you could try b

[PATCH] D71572: [ItaniumCXXABI] Use linkonce_odr instead of weak_odr for tls wrappers on Windows

rnk added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2519-2520 return VarLinkage; + // On Windows, WeakODR is a no-op, boiling down to the same as normal external + // linkage. + if (CGM.getTriple().isOSWindows()) mstorsjo wrote:

[PATCH] D71572: [ItaniumCXXABI] Use linkonce_odr instead of weak_odr for tls wrappers on Windows

rnk added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2519-2520 return VarLinkage; + // On Windows, WeakODR is a no-op, boiling down to the same as normal external + // linkage. + if (CGM.getTriple().isOSWindows()) I would say that

[PATCH] D71400: [RFC] [MinGW] Implicitly add .exe suffix if not provided

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71400/new/ https://reviews.llvm.org/D71400 ___ cfe-commits mailing list cfe-commit

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

rnk added a comment. In D70527#1785552 , @ikudrin wrote: > Personally, I would prefer to see the file name and path to be changed as > little as possible because that would help to recognize the files better. We > cannot use `remove_dots()` on POSIX OSes

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

rnk added a comment. From auditing the call sites, it seems that almost all of them could be simplified by using the new API: http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp/rgetCanonicalName Comment at: clang/include/clang/Basic/FileManager.h:226 /// The can

[PATCH] D71320: [IR] Split out target specific intrinsic enums into separate headers

rnk added a comment. In D71320#1784726 , @thakis wrote: > I think this broke the reverse-iteration bot: > http://lab.llvm.org:8011/builders/reverse-iteration/builds/15619 Looks like I forgot to build polly, and @aheejin fixed it. Thanks! I usually enab

[PATCH] D71434: [Driver] Use .init_array for all gcc installations and simplify Generic_ELF -fno-use-init-array rules

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm shipit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D71439: Allow redeclaration of __declspec(uuid)

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Seems reasonable, looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71439/new/ https://reviews.llvm.org/D71439 __

[PATCH] D71039: Add support for the MS qualifiers __ptr32, __ptr64, __sptr, __uptr.

rnk added a comment. Sorry for the delay, overall this seems like the right approach. Comment at: clang/include/clang/AST/Type.h:477-479 + ((isPtrSizeAddressSpace(A) && B == LangAS::Default) || +(isPtrSizeAddressSpace(B) && A == LangAS::Default) || +

[PATCH] D71434: [Driver] Use .init_array for all gcc installations and simplify Generic_ELF -fno-use-init-array rules

rnk added a comment. I think we should do this, but let's add a release note for it. There's a flag, so users always have a workaround if clang starts doing the wrong thing for them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71434/new/ https:

[PATCH] D71427: Move TypeSourceInfo to Type.h

rnk created this revision. rnk added a reviewer: rsmith. Herald added a project: clang. TypeSourceInfo is a thin wrapper around TypeLocs. Notionally, the best place for it to live would be TypeLoc.h, but Decl.h requires it to be complete, so it needs to be lower in the dependency graph. Type.h see

[PATCH] D71400: [RFC] [MinGW] Implicitly add .exe suffix if not provided

rnk added inline comments. Comment at: clang/lib/Driver/ToolChains/MinGW.cpp:164 + const char *OutputFile = Output.getFilename(); +#ifdef _WIN32 + if (!llvm::sys::path::filename(OutputFile).contains('.')) Can you add what you wrote in the commit message as a co

[PATCH] D71320: [IR] Split out target specific intrinsic enums into separate headers

rnk added a comment. In D71320#1780805 , @thakis wrote: > Any reason these are called .h? All our other tablegen outputs are called > .inc. Yes, they have header guards, they are not textual. Most other tablegen outputs are intended to be used with som

[PATCH] D71393: Default to -fuse-init-array

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. +1, the list of exceptions that will use .ctors is smaller than the list of platforms that default to init_array. And in general, I support anything that reduces the length of -cc1 command lines for

[PATCH] D71320: [IR] Split out target specific intrinsic enums into separate headers

rnk marked 2 inline comments as done. rnk added a comment. Thanks, I split off the rename and landed it separately, since it is more mechanical. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71320/new/ https://reviews.llvm.org/D71320 __

[PATCH] D71320: [IR] Split out target specific intrinsic enums into separate headers

This revision was automatically updated to reflect the committed changes. Closed by commit rG5d986953c8b9: [IR] Split out target specific intrinsic enums into separate headers (authored by rnk). Changed prior to commit: https://reviews.llvm.org/D71320?vs=233231&id=233480#toc Repository: rG L

[PATCH] D69471: [Coverage] Revise format to reduce binary size

rnk added a comment. I have a Windows build directory and am motivated to debug this. I'll try to do it tomorrow, but I have a couple of other deadlines so I can't make a very firm promise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69471/new/

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

rnk added a comment. I think we should remove it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70854/new/ https://reviews.llvm.org/D70854 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

rnk added a comment. I see there is no setfacl on Mac. Personally, I feel like this test doesn't have much value. Rafael added it long ago when we changed how we opened files in some way that I guess affected umask. I'm not sure we really need this regression test given how unportable it seems

[PATCH] D71320: [IR] Split out target specific intrinsic enums into separate headers

rnk marked an inline comment as done. rnk added inline comments. Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:759 Type *Ty) const; - int getIntImmCost(Intrinsic::ID IID, unsigned Idx, const APInt &Imm, -Type *Ty) const; +

[PATCH] D71320: [IR] Split out target specific intrinsic enums into separate headers

rnk created this revision. rnk added reviewers: efriedma, echristo. Herald added subscribers: cfe-commits, luismarques, apazos, sameer.abuasal, pzheng, s.egerton, lenary, dmgreen, Petar.Avramovic, jsji, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, atanasyan, edward-jones, zzheng, M

[PATCH] D71313: [AST] Split parent map traversal logic into ParentMapContext.h

rnk created this revision. rnk added reviewers: bkramer, klimek, rsmith. Herald added subscribers: lldb-commits, kbarton, mgorny, nemanjai. Herald added projects: clang, LLDB. The only part of ASTContext.h that requires most AST types to be complete is the parent map. Nothing in Clang proper uses

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

rnk updated this revision to Diff 232975. rnk added a comment. - rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65543/new/ https://reviews.llvm.org/D65543 Files: clang/docs/ReleaseNotes.rst clang/include/clang/Driver/ToolChain.h clang/

[PATCH] D70627: [AST] Split out Attrs.h for table generated *Attr classes

rnk abandoned this revision. rnk added a comment. I think this isn't necessary. I think I got most of the value of this in rG60573ae6fe509b618dc6a2c5c55d921bccd77608 , and we don't need two AttrBase.h / Attr.h headers in the l

[PATCH] D71222: Include Stmt.h where it seems to be necessary for modules builds

rnk created this revision. rnk added a reviewer: rdhindsa. Herald added a project: clang. rdhindsa accepted this revision. rdhindsa added a comment. This revision is now accepted and ready to land. LGTM After 60573ae6fe50 rem

[PATCH] D71222: Include Stmt.h where it seems to be necessary for modules builds

This revision was automatically updated to reflect the committed changes. Closed by commit rG1164d43855fd: Include Stmt.h where it seems to be necessary for modules builds (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71222/new/ h

[PATCH] D71159: [Attr] Move ParsedTargetAttr out of the TargetAttr class

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

[PATCH] D68627: [Sema][X86] Consider target attribute into the checks in validateOutputSize and validateInputSize.

rnk added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:2826 + /// valid feature names. + TargetAttr::ParsedTargetAttr + filterFunctionTargetAttrs(const TargetAttr *TD) const; I reverted this because it requires TargetAttr to be complete he

[PATCH] D70849: [AST] Traverse the class type loc inside the member pointer type loc.

rnk added a comment. We also hit this, and I have a repro here, but it depends on the Chromium Blink GC plugin: https://bugs.chromium.org/p/chromium/issues/detail?id=1031274#c4 I think you just need an RAV client that walks over some particular AST node to trigger the assert. I assume there is n

[PATCH] D68627: [Sema][X86] Consider target attribute into the checks in validateOutputSize and validateInputSize.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Hoisting this to ASTContext and using it where needed seems like the logical thing to do. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68627/new/ https://reviews.llvm.org/D68627

[PATCH] D65761: Add Windows Control Flow Guard checks (/guard:cf).

rnk added a comment. In D65761#1766993 , @dmajor wrote: > Are there any plans to implement `__declspec(guard(nocf))` or an equivalent > mechanism? `__attribute__((nocf_check))` doesn't do anything without the > -fcf-protection flag. (https://bugs.llvm.or

[PATCH] D71098: Handle two corner cases in creduce-clang-crash.py

This revision was automatically updated to reflect the committed changes. Closed by commit rG1f822f212cde: Handle two corner cases in creduce-clang-crash.py (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71098/new/ https://reviews.

[PATCH] D71098: Handle two corner cases in creduce-clang-crash.py

rnk created this revision. rnk added a reviewer: akhuang. Herald added a project: clang. First, call os.path.normpath on the filename argument. I passed in ./foo-asdf.cpp, and this meant that the script failed to find the filename, and bad things happened. Second, call os.path.abspath on binaries

[PATCH] D70701: Fix more VFS tests on Windows

rnk added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078 +std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl &Path) const { + if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) || + llvm::sys::path::is_abso

[PATCH] D71092: [VFS] Use path canonicalization on all paths

rnk added a comment. I would add, take a look at the existing unit tests (llvm/unittests/Support/VirtualFileSystemTests.cpp) and audit for _WIN32 ifdefs that we can remove after this change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71092/new/ https://reviews.llvm.org/D71092 __

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

rnk added a comment. I'd be fine with the cmake var and the env var and no driver flag. I think clang developers on Darwin probably want the single-process `clang -c` behavior (that's how I interpreted what Duncan said), even though Apple as a distributor of clang wants the multi-process behavi

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

rnk added a comment. In D69950#1770138 , @mstorsjo wrote: > This (when reapplied in > https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of > code that earlier built fine. A reduced example: > > namespace glslang { > class TPoo

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rnk added a comment. In D70172#1746451 , @yaxunl wrote: > We are not using Itanium ABI when we do host compilation of CUDA/HIP on > windows. During the host compilation on windows only MS C++ ABI is used. > > This issue is not due to mixing MS ABI with It

[PATCH] D70615: Add an -fno-temp-file flag for compilation

rnk added a comment. +more reviewers This doesn't add any code complexity, we already have the boolean UseTempFile flag, so I think we should do this. I would also point out that right now, in my LLVM build directory on Windows, I have 895 *.obj.tmp files: $ find . -iname '*.obj.tmp' | wc -l

[PATCH] D70726: [OpenMP50] Add parallel master construct

rnk added a comment. One of the new tests doesn't pass on Windows, so I reverted this in rG33f6d465d790ac5c9b949e6bc05127d356212079 . I made an attempt to fix the checks, but it wasn't trivial. Repository: rG LLVM Github M

[PATCH] D70996: Bug 43965 - Value of _MSVC_LANG doesn't match MSVC++ VS2019 /std:c++latest mode

This revision was automatically updated to reflect the committed changes. Closed by commit rG9c29aed6980d: Bug 43965 - Value of _MSVC_LANG doesn't match MSVC++ VS2019 /std:c++latest mode (authored by Manna, committed by rnk). Herald added a project: clang. Herald added a subscriber: cfe-commits.

[PATCH] D67463: [MS] Warn when shadowing template parameters under -fms-compatibility

rnk added a comment. In D67463#1769238 , @tstellar wrote: > In D67463#1767919 , @rnk wrote: > > > @hans, are we still accepting 9.0.1 patches? I thought we'd already made a > > release candidate. > > > I'm still ac

[PATCH] D71012: Also check /Fo when deciding on the .gcna / .gcda filename (PR44208)

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/Driver/ToolChains/Clang.cpp:911 // bitcode file in the case of LTO. // FIXME: There should be a simpler way to find the object file for this //

[PATCH] D69876: Allow output constraints on "asm goto"

rnk added a subscriber: rsmith. rnk added a comment. Changes seem fine to me, FWIW. +@rsmith Comment at: clang/test/Analysis/uninit-asm-goto.cpp:10 +return -1; +} This could use a test for the case where an input is uninitialized, and where an uninitialize

[PATCH] D70905: Actually delay processing DelayedDllExportClasses until the outermost class is finished (PR40006)

rnk added a comment. It occurs to me that this will cause some strange ordering in some cases. Consider: namespace pr40006 { // Delay emitting the method also past the instantiation of Tmpl, i.e. // until the top-level class Outer is completely finished. template struct Tmpl {}; struct

[PATCH] D70905: Actually delay processing DelayedDllExportClasses until the outermost class is finished (PR40006)

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I'll add, looks good with suggested fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70905/new/ https://reviews.llvm.org/D70905 ___ cfe-comm

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/Misc/permissions.cpp:8 // RUN: umask 002 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t aganea wrote: > rnk wrote: > > If you change thi

[PATCH] D70931: [MS] Emit exported complete/vbase destructors

This revision was automatically updated to reflect the committed changes. Closed by commit rG705a6aef3502: [MS] Emit exported complete/vbase destructors (authored by rnk). Changed prior to commit: https://reviews.llvm.org/D70931?vs=231796&id=231998#toc Repository: rG LLVM Github Monorepo CH

[PATCH] D70931: [MS] Emit exported complete/vbase destructors

rnk marked an inline comment as done. rnk added inline comments. Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1349 + // the base dtor is emitted. + // FIXME: To match MSVC, this should only be done when the class was + // dllexported inlines are being exported. --

[PATCH] D67463: [MS] Warn when shadowing template parameters under -fms-compatibility

rnk added a comment. @hans, are we still accepting 9.0.1 patches? I thought we'd already made a release candidate. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67463/new/ https://reviews.llvm.org/D67463 ___ cfe-comm

[PATCH] D70931: [MS] Emit exported complete/vbase destructors

rnk created this revision. rnk added a reviewer: hans. Herald added a project: clang. Fixes PR44205 I checked, and deleting destructors are not affected. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D70931 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/CodeGenCX

[PATCH] D70302: [CodeGen] Fix clang crash on aggregate initialization of array of labels

rnk added a comment. In D70302#1764261 , @johannes wrote: > I see, apologies for the premature commit. No problem, LLVM's code review practices are somewhat opaque. Comment at: clang/test/CodeGen/label-array-aggregate-init.c:1 +// RUN

[PATCH] D70854: [Clang] In tests, do not always assume others permissions are set

rnk added inline comments. Comment at: clang/test/Misc/permissions.cpp:8 // RUN: umask 002 // RUN: %clang_cc1 -emit-llvm-bc %s -o %t If you change this to `umask 022`, does that result in `rw-r-`? That would make the test meaningful on your system. Rep

[PATCH] D70701: Fix more VFS tests on Windows

rnk added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078 +std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl &Path) const { + if (llvm::sys::path::is_absolute(Path, llvm::sys::path::Style::posix) || + llvm::sys::path::is_abso

[PATCH] D70302: [CodeGen] Fix clang crash on aggregate initialization of array of labels

rnk added a comment. Fix looks good post commit, but we should enhance the test. Comment at: clang/test/CodeGen/label-array-aggregate-init.c:1 +// RUN: %clang -cc1 -emit-llvm %s -o /dev/null + It's best practice to filecheck for something, even if this used to

[PATCH] D70791: Workaround for MSVC 16.3.* pre-c++17 type trait linkage

rnk added a comment. My version of cl.exe claims to have version 19.23.28107 and my headers are in Tools/MSVC/14.23.28105/include, and I see that this is fixed in xtr1common. is_integral_v and the helpers are defined with implicit template instantiations. There are no fully specialized template

[PATCH] D70791: Workaround for MSVC 16.3.* pre-c++17 type trait linkage

rnk added a subscriber: thakis. rnk added a comment. I thought we agreed to remove the workaround that we had for this and just declare that the old MSVC STL release was buggy and users should update: https://bugs.llvm.org/show_bug.cgi?id=42027 It's a point release that preserves ABI compatibilit

[PATCH] D70627: [AST] Split out Attrs.h for table generated *Attr classes

rnk marked an inline comment as done. rnk added inline comments. Comment at: clang/include/clang/AST/Attrs.h:1 +//===--- Attr.h - Classes for representing attributes --*- C++ -*-===// +// aaron.ballman wrote: > rnk wrote: > > aaron.ballman wrote: > > > rn

[PATCH] D70627: [AST] Split out Attrs.h for table generated *Attr classes

rnk marked an inline comment as done. rnk added inline comments. Comment at: clang/include/clang/AST/Attrs.h:1 +//===--- Attr.h - Classes for representing attributes --*- C++ -*-===// +// aaron.ballman wrote: > rnk wrote: > > aaron.ballman wrote: > > > Th

[PATCH] D70627: [AST] Split out Attrs.h for table generated *Attr classes

rnk marked an inline comment as done. rnk added a comment. Thanks! Comment at: clang/include/clang/AST/Attrs.h:1 +//===--- Attr.h - Classes for representing attributes --*- C++ -*-===// +// aaron.ballman wrote: > This should read `Attrs.h` instead, but

[PATCH] D70627: [AST] Split out Attrs.h for table generated *Attr classes

rnk created this revision. rnk added a reviewer: aaron.ballman. Herald added subscribers: arphaman, mgrang, kosarev, jholewinski. Herald added a reviewer: jdoerfert. Herald added a project: clang. Many AST headers require Attr to be complete, but do not need the complete table generated list of at

[PATCH] D70467: [Distro] Bypass distro detection on non-Linux hosts

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. In D70467#1755364 , @aganea wrote: > Actually, I'm not sure the `DetectDistro()` does what it intends to do when > cross-compiling: if I compile on Ubuntu an

[PATCH] D70518: [clang-include-fixer] Suppress cmd prompt from Vim on Windows

This revision was automatically updated to reflect the committed changes. Closed by commit rGe1e7b6f381a9: [clang-include-fixer] Suppress cmd prompt from Vim on Windows (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70518/new/ http

[PATCH] D70518: [clang-include-fixer] Suppress cmd prompt from Vim on Windows

[PATCH] D70467: [Distro] Bypass distro detection on Windows

rnk added a comment. I'd rather avoid the ifdef if possible. I looked to see where these come from: $ git grep -l Distro ../clang/lib/Driver/ ../clang/lib/Driver/CMakeLists.txt ../clang/lib/Driver/Distro.cpp ../clang/lib/Driver/ToolChains/Clang.cpp ../clang/lib/Driver/ToolChains/Cuda.cp

[PATCH] D70149: [C-index] Fix annotate-deep-statements test in Debug target

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I was wondering if Richard would add something since he created this stack size management code, but in the absence of that, this looks right to me. Repository: rG LLVM Github Monorepo CHA

[PATCH] D70340: Add a key method to Sema to optimize debug info size

This revision was automatically updated to reflect the committed changes. Closed by commit rG586f65d31f32: Add a key method to Sema to optimize debug info size (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://revie

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk marked an inline comment as done. rnk added inline comments. Comment at: clang/include/clang/Sema/Sema.h:335 + /// A key method to reduce duplicate type info from Sema. + virtual void anchor(); hans wrote: > I worry that this is going to look obscure to m

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk updated this revision to Diff 230130. rnk added a comment. - add final, tweak comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://reviews.llvm.org/D70340 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. Oh, yeah, I forgot this causes tons of -Wdelete-non-virtual-dtor warnings, so I'll have to look into that before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://reviews.llvm.org/D70340

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. In D70340#1751148 , @hans wrote: > Nice! > > Silly questions, but for my own education: I thought the key function concept > only existed in the Itanium ABI, but from your numbers it sounds like it's a > concept, at least for debug i

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk updated this revision to Diff 229943. rnk added a comment. - comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://reviews.llvm.org/D70340 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp Index: clang/lib

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. In D70340#1749073 , @thakis wrote: > With the overhead being the cost of a single vtable with one entry? Or is > there more? I guess I worry about the extra dead vtable pointer in Sema. But, I don't think it matters. I think we sho

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

rnk added a comment. We already have llvm-rc.exe which one could plausibly use to compile rc files, and I didn't like `binary.lower().endswith('rc.exe')` as a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70196/new/ https://reviews.llvm.org/

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. In D70340#1748712 , @thakis wrote: > I don't see any reason not to do this. What's there to discuss? I'm probably > missing something obvious. I guess I was thinking about enabling this only in +asserts builds, so we pay zero overh

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk created this revision. rnk added reviewers: dblaikie, hans, thakis, rsmith. Herald added a subscriber: aprantl. Herald added a project: clang. DONOTSUBMIT, this patch is just for the purpose of discussion. It turns out that the debug info describing the Sema class is an appreciable percentage

[PATCH] D70280: Remove Support/Options.h, it is unused

This revision was automatically updated to reflect the committed changes. Closed by commit rG631be5c0d411: Remove Support/Options.h, it is unused (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70280/new/ https://reviews.llvm.org/D7

[PATCH] D70280: Remove Support/Options.h, it is unused

rnk created this revision. rnk added a reviewer: beanz. Herald added subscribers: cfe-commits, hiraditya, mgorny. Herald added a reviewer: jfb. Herald added projects: clang, LLVM. It was added in 2014 in 732e0aa9fb84f1 with one use in Scalarizer.cpp. That one use was then removed when porting to t

[PATCH] D70101: [X86] Fix the implementation of __readcr3/__writecr3 to work in 64-bit mode

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

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Related to this discussion, I noticed `to_vector` in STLExtras.h: https://github.com/llvm/llvm-project/blob/e2369fd197d9e/llvm/include/llvm/ADT/STLExtras.h#L1329 I guess that would be the most i

[PATCH] D70101: [X86] Fix the implementation of __readcr3/__writecr3 to work in 64-bit mode

rnk added a comment. In D70101#1741565 , @craig.topper wrote: > So in order to match the long and int behavior, I need two different macros > to replace the argument? I think __INTPTR_TYPE__ will return int on 32-bit > targets so I can't use that for th

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

This revision was automatically updated to reflect the committed changes. Closed by commit rGe2369fd197d9: [clang-include-fixer] Skip .rc files when finding symbols (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70196/new/ https://

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

rnk added a comment. This is a single .rc entry: { "directory": "C:/src/llvm-project/build", "command": "C:\\PROGRA~2\\WI3CF2~1\\10\\bin\\100183~1.0\\x64\\rc.exe -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_ DEPRECATE -D_CRT_SE

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rnk added a comment. Are we sure using both Itanium and MS C++ ABIs at the same time is really the best way forward here? What are the constraints on CUDA that require the Itanium ABI? I'm sure there are real reasons you can't just use the MS ABI as is, but I'm curious what they are. Was there

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

rnk created this revision. rnk added reviewers: sammccall, bkramer. Herald added a project: clang. For some reason CMake includes entries for .rc files, but find-all-symbols handles them improperly. See PR43993 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D70196 Files: cl

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

rnk added a subscriber: dblaikie. rnk added a comment. For reference, this is the code sequence that needs the SmallVector change: https://github.com/llvm/llvm-project/blob/99e2cba219aea80b3f11de2aa4e0192b28852de4/clang/lib/Frontend/CompilerInvocation.cpp#L1872 Comment at: clan

[PATCH] D69766: [Clang][MSVC] Use GetLinkerPath like the other toolchains for consistency

rnk added a comment. In D69766#1739060 , @Ericson2314 wrote: > Reading https://llvm.org/docs/DeveloperPolicy.html it looks like I need to > submit an patch email? Is that true, or can/will someone with perms see the > approval and eventually merge it fro

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

rnk added a comment. I see. You know, these things would be nicely handled if we just had a reliable asynch SEH implementation. >_> What do you think of making `FrontendOptions::Inputs` be a `SmallVector<*, 0>`? It's janky and inconsistent with the rest of that struct, but it seems less invasi

[PATCH] D70101: [X86] Fix the implementation of __readcr3/__writecr3 to work in 64-bit mode

rnk added a comment. What do you think of using `__INTPTR_TYPE__` for this instead? If that doesn't work, we could define and undefine our own macro to use as a typedef. The resulting code should be clean enough that we can generalize it to `__readcr0` etc. It looks like `long` is the correct

[PATCH] D69969: [SEH] Defer checking filter expression types until instantiaton

This revision was automatically updated to reflect the committed changes. Closed by commit rG7177ce978e8f: [SEH] Defer checking filter expression types until instantiaton (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69969/new/ ht

[PATCH] D69969: [SEH] Defer checking filter expression types until instantiaton

rnk created this revision. rnk added a reviewer: amccarth. Herald added a project: clang. While here, wordsmith the error a bit. Now clang says: error: filter expression has non-integral type 'Foo' Fixes PR43779 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D69969 Files:

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I don't have experience with clang-tidy tests, but I think these two small, test-only changes are small enough that you can go ahead and commit them without review from a clang-tidy owner. lgtm CH

[PATCH] D69958: Improve VFS compatibility on Windows

rnk added a comment. This looks good to me. I had one suggestion, and I would also like to hear from another reviewer who has more ownership over VFS. Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:657 + // slashes to match backslashes (and vice versa). + bool pat

[PATCH] D69766: [Clang][MSVC] Use GetLinkerPath like the other toolchains for consistency

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

[PATCH] D69763: [Clang][Test]: Remaining "lld-link2" -> "lld-link"

rnk accepted this revision. rnk added a comment. In D69763#1733382 , @Ericson2314 wrote: > I am curious, how did this work since there is no longer an `lld-link2`? Were > these tests failing or not being run? These tests don't actually need to run the l

<    4   5   6   7   8   9   10   11   12   13   >