[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] 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] D70518: [clang-include-fixer] Suppress cmd prompt from Vim on Windows

[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] 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] 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] 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 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 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 a comment. Looks like @rsmith did this here: https://github.com/llvm/llvm-project/commit/fbe2369f1a514423e4c25417ab3532502fde6f2a I see that it was replacing a CHECK for a specific comdat group with no comdat at all. I *think* that's not correct, we should be checking for the trivial

[PATCH] D71650: [AST] Fix compilation with GCC < 8 for MinGW

rnk added a comment. I agree, in this case I think the comment will just be extra noise. The comment is likely to outlive the use of these buggy GCC versions. If someone runs into this bug again, it will probably happen somewhere else far away from this code, and the comment isn't going to help

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

rnk planned changes to this revision. rnk marked an inline comment as done. rnk added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:653 template DynTypedNodeList getParents(const NodeT &Node) { return getParents(ast_type_traits::DynTypedNode::create(N

[PATCH] D71650: [AST] Fix compilation with GCC < 8 for MinGW

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Pushing code review approval button. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71650/new/ https://reviews.llvm.org/D71650 _

[PATCH] D71427: Move TypeSourceInfo to Type.h

This revision was automatically updated to reflect the committed changes. Closed by commit rG71f9c30b5348: Move TypeSourceInfo to Type.h (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71427/new/ https://reviews.llvm.org/D71427 Fil

[PATCH] D71677: [ms] [X86] Use "P" modifier on operands to call instructions in inline X86 assembly.

rnk added a comment. Where is {0:P} actually documented? I don't see it in LangRef, but I do see it in the code. Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2871 + // differently when referenced in MS-style inline assembly. + if (Name.startswith("call") || Nam

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/Basic/FileManager.cpp:551 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) { // FIXME: use llvm::sys::fs::canonical() when it gets

[PATCH] D71677: [ms] [X86] Use "P" modifier on operands to call instructions in inline X86 assembly.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2871 + // differently when referenced in MS-style inline assembly. + if (Name.startswith("call") || Name.startswit

[PATCH] D41910: [Concepts] Constrained partial specializations and function overloads.

rnk added a comment. This was relanded and reverted again today (d3f5769d5e93b30d4a8b4696381d5e4a304992fa & 79cc9e9b304a90598e8def4c8b5354d1f99186eb ). I g

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

rnk added a comment. Three of these tests have been failing for me locally since this patch landed: Failing Tests (3): Clang :: CodeGen/debug-info-extern-basic.c Clang :: CodeGen/debug-info-extern-duplicate.c Clang :: CodeGen/debug-info-extern-multi.c I suspect bots do not s

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

rnk added a comment. I wasn't able to fix forward because the functionality you added doesn't seem to work, at least not for me, so I went ahead and reverted the change. I considered XFAILing the test, but I am concerned that may cause it to pass unexpectedly on some bot somewhere. In any case,

[PATCH] D71818: reland "[DebugInfo] Support to emit debugInfo for extern variables"

rnk added inline comments. Comment at: clang/test/CodeGen/debug-info-extern-basic.c:1 +// REQUIRES: bpf-registered-target +// RUN: %clang_cc1 -x c -debug-info-kind=limited -triple bpf-linux-gnu -emit-llvm %s -o - | FileCheck %s Clang does not require a registere

[PATCH] D71818: reland "[DebugInfo] Support to emit debugInfo for extern variables"

rnk added a comment. I patched this in, and the tests don't pass for me locally, so we're back to where we started. Go ahead and land it, I guess. I'm kind of not happy that we had to poke another virtual method through the ASTConsumer interface to make this happen. Repository: rG LLVM Gith

[PATCH] D71818: reland "[DebugInfo] Support to emit debugInfo for extern variables"

rnk added subscribers: rjmccall, rsmith. rnk added a comment. +@rsmith @rjmccall For more meaningful feedback on the code structure. Comment at: clang/include/clang/Sema/Sema.h:671-672 + /// All the external declarations encoutered and used in the TU. + SmallVector Externa

[PATCH] D71818: reland "[DebugInfo] Support to emit debugInfo for extern variables"

rnk added a comment. You forgot to update MultiplexConsumer: diff --git a/clang/include/clang/Frontend/MultiplexConsumer.h b/clang/include/clang/Frontend/MultiplexConsumer.h index ca6ed8310ae..3054e184281 100644 --- a/clang/include/clang/Frontend/MultiplexConsumer.h +++ b/clang/include/c

[PATCH] D71818: reland "[DebugInfo] Support to emit debugInfo for extern variables"

rnk added a comment. Ah, I see, these tests will fail if you link in some clang plugins, which I do for Chrome. So, this only affects me. Anyway, I'll fix it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71818/new/ https://reviews.llvm.org/D71818

[PATCH] D71716: [ItaniumCXXABI] Don't mark an extern_weak init function as dso_local on windows

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm TLS thread wrappers seem to reimplement a lot of the logic that would happen naturally for a regular function declaration. I wonder if there is some way to restructure the code to have less du

[PATCH] D75708: Add warnings for casting ptr -> smaller int for C++ in Microsoft mode

rnk added a comment. Looks good, but we should add a test. I can push this for you and even give attribution (hooray, git!), but I think you have enough uploaded code reviews that you could request push access as described here: https://llvm.org/docs/DeveloperPolicy.html#new-contributors Link t

[PATCH] D75708: Add warnings for casting ptr -> smaller int for C++ in Microsoft mode

This revision was automatically updated to reflect the committed changes. Closed by commit rGcfff4851acc5: Add warnings for casting ptr -> smaller int for C++ in Microsoft mode (authored by aeubanks, committed by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D75708: Add warnings for casting ptr -> smaller int for C++ in Microsoft mode

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Thanks, looks good! I'll push it in a sec. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75708/new/ https://reviews.llvm.org/D75708 ___

[PATCH] D75723: [X86] Make intrinsics _BitScan* not limited to Windows

rnk added inline comments. Comment at: clang/include/clang/Basic/BuiltinsX86.def:1904 +// BITSCAN +TARGET_BUILTIN(_BitScanForward, "UcUNi*UNi", "n", "") +TARGET_BUILTIN(_BitScanReverse, "UcUNi*UNi", "n", "") craig.topper wrote: > skan wrote: > > craig.topper wrot

[PATCH] D75752: [Sema] Move pointer to int cast warnings under -Wmicrosoft-cast

rnk added a comment. I agree with the reasoning, but this is likely to fire all over existing C++ code. In fact, the one we just added finds issues in Chrome https://ci.chromium.org/p/chrome/builders/ci/ToTWin/5879 ../../base/debug/close_handle_hook_win.cc(155,16): error: cast to smaller integer

[PATCH] D75643: [Sema] Don't emit pointer to int cast warnings under -Wmicrosoft-cast

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I re-read Richard's comment: > Would it make sense to put the MS extension warning into the > -Wpointer-to-int-cast group so that we can control this warning consistently > across platforms? You co

[PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

rnk created this revision. rnk added reviewers: aaron.ballman, hans. Herald added projects: clang, LLDB. Herald added a subscriber: lldb-commits. Module.h takes 86ms to parse, mostly parsing the class itself. Avoid it if possible. ASTContext.h depends on ExternalASTSource.h. A few NFC changes wer

[PATCH] D75643: [Sema] Don't emit pointer to int cast warnings under -Wmicrosoft-cast

rnk added a comment. I think I patched this in, ran tests, but didn't push. Feel free to push if you get the chance. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75643/new/ https://reviews.llvm.org/D75643 __

[PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

rnk added a comment. In D75784#1910688 , @aprantl wrote: > This will no doubt also need some patches to the Swift compiler, but given > the NFC-ness this hopefully should be fine. True. I could leave behind a typedef for the ASTSourceDescriptor if it ma

[PATCH] D75406: Avoid including FileManager.h from SourceManager.h

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

[PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

rnk added a comment. Thanks! In D75784#1917181 , @aprantl wrote: > To avoid bot breakage, I would recommend testing -DLLVM_ENABLE_MODULES=1 > stage2 build works before landing this, as it is notoriously sensitive to > header reshuffling. Good idea, I

[PATCH] D75784: Avoid including Module.h from ExternalASTSource.h

This revision was automatically updated to reflect the committed changes. rnk marked an inline comment as done. Closed by commit rGc915cb957dc3: Avoid including Module.h from ExternalASTSource.h (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

<    10   11   12   13   14   15   16   17   18   19   >