[PATCH] D50055: Update the coding standard about NFC changes and whitespace

JDevlieghere added inline comments. Comment at: docs/CodingStandards.rst:514-516 +line of code. Some common editors will automatically remove trailing whitespace +when saving a file which causes unrelated changes to appear in diffs and +commits. chandlerc wrote:

[PATCH] D49549: Change 'clang-test' to 'check-clang' on the hacking webpage

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Looks good, thanks! PS: I don't think it hurts, but it's sufficient to add cfe-commits as a subscriber. I believe it's an automation account (so it won't be able to accept on phab

[PATCH] D46694: [diagtool] Install diagtool

JDevlieghere created this revision. JDevlieghere added reviewers: arphaman, dexonsmith, jkorous, rsmith. Herald added a subscriber: mgorny. Although not very well known, diagtool is an incredibly convenient utility for dealing with diagnostics. I believe it's worth adding this to the install tar

[PATCH] D46694: [diagtool] Install diagtool

JDevlieghere added a comment. In https://reviews.llvm.org/D46694#1094617, @dexonsmith wrote: > This SGTM, but I wouldn't mind hearing from others. I wonder if this is > worth a quick RFC on cfe-dev? Sounds good, I've created a thread here: http://lists.llvm.org/pipermail/cfe-dev/2018-May/057

[PATCH] D46694: [diagtool] Install diagtool

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL332448: [diagtool] Add diagtool to install target. (authored by JDevlieghere, committed by ). Changed prior to commit:

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Thanks David, LGTM https://reviews.llvm.org/D45686 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-

[PATCH] D47190: [Driver] Add -ivfsoverlay-lib option to load VFS from shared library

JDevlieghere added a comment. I don't know much about the VFS so I only did a superficial review. Comment at: include/clang/Lex/HeaderSearchOptions.h:176 + /// \brief The set of libraries with user-provided virtual filesystem. + std::vector VFSOverlayLibs;

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added a comment. I very much like this check. I only have a few minor comments, but maybe this encourages others to have a look too! Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:85 +diag(ElseIfWithoutElse->getLocStart(), + "potential unco

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:96 +// Only the default branch (we explicitly matched for default!) exists. +if (CaseCount == 1) { + diag(SwitchWithDefault->getLocStart(), JonasToth wrote

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:76 + +std::size_t twoPow(std::size_t Bits) { + const std::size_t DiscreteValues = 1ul << Bits; Add a comment describing what this function does. I'd move and rephrase

[PATCH] D37808: [clang-tidy] Add new hicpp-multiway-paths-covered check for missing branches

JDevlieghere added inline comments. Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:80 + + // Unsigned overflow occured. Returning max() is sufficient, since noone + // writes so many case labels in source code. Maybe merge this with the function com

[PATCH] D51485: [Sema][NFC] Trivial cleanup in ActOnCallExpr

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Nice cleanup, LGTM. Repository: rC Clang https://reviews.llvm.org/D51485 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Assuming `ArgExprs` doesn't change between those two calls this seems fine. Comment at: Sema/SemaExpr.cpp:5338 Context.DependentTy, VK_RValue, RParen

[PATCH] D42370: Issue local statics in correct DWARF lexical scope

JDevlieghere accepted this revision as: JDevlieghere. JDevlieghere added a comment. This revision is now accepted and ready to land. This makes sense to me, LGTM. https://reviews.llvm.org/D42370 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D51807: Remove all uses of DIFlagBlockByrefStruct from Clang

JDevlieghere accepted this revision as: JDevlieghere. JDevlieghere added a comment. This revision is now accepted and ready to land. Thanks Adrian, LGTM! https://reviews.llvm.org/D51807 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://

[PATCH] D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?

JDevlieghere added inline comments. Comment at: Sema/SemaExpr.cpp:5338 Context.DependentTy, VK_RValue, RParenLoc); } else { jkorous wrote: > JDevlieghere wrote: > > While you're at it you might as well remove the else branch here. > Sorry, I

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

JDevlieghere created this revision. JDevlieghere added reviewers: aprantl, dexonsmith. We generate incorrect values for the DW_AT_data_bit_offset for interfaces in Objective-C. I can only speculate as to what we were trying to achieve by taking the modulo of the bit size with the byte size, but

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

JDevlieghere added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369 + ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field) + : 0; } else { aprantl wrote: > aprantl wrote: > > It might help to attempt so

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

JDevlieghere added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369 + ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field) + : 0; } else { JDevlieghere wrote: > aprantl wrote: > > aprantl wrote: > > > I

[PATCH] D51990: [DebugInfo] Fix emitting of bit offset for ObjC

JDevlieghere added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2369 + ? CGM.getObjCRuntime().ComputeBitfieldBitOffset(CGM, ID, Field) + : 0; } else { aprantl wrote: > JDevlieghere wrote: > > JDevlieghere wrote: >

[PATCH] D52058: Add Parameters to DW_AT_name Attribute of Template Variables

JDevlieghere added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3126 + } else { +templateParameters = nullptr;//llvm::DINodeArray().get(); + } What's the meaning of this comment? Comment at: lib/CodeGen/CGDebugInfo.h:654 +

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

JDevlieghere created this revision. JDevlieghere added reviewers: labath, dblaikie, probinson. Currently, support for debug_types is only present for ELF and trying to pass -fdebug-types-section for other targets results in a crash in the backend. Until this is fixed, we should emit a diagnostic

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

JDevlieghere added a comment. Please see PR38190 for more details. Repository: rC Clang https://reviews.llvm.org/D49594 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

JDevlieghere updated this revision to Diff 156471. JDevlieghere added a comment. Thanks, I meant to use `isOSBinFormatELF` but I think it got lost in an accidental undo. The test didn't capture that because it's using a linux target triple. https://reviews.llvm.org/D49594 Files: clang/lib/D

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

JDevlieghere added a comment. In https://reviews.llvm.org/D49594#1169809, @probinson wrote: > Is this because type units depend on COMDAT support? I had a vague idea that > COFF also supports COMDAT. It's more that I want to reflect the current situation and prevent MC from crashing while we

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

This revision was automatically updated to reflect the committed changes. Closed by commit rL337717: [DebugInfo] Error out when enabling -fdebug-types-section on non-ELF target. (authored by JDevlieghere, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https:/

[PATCH] D42766: [DebugInfo] Support DWARFv5 source code embedding extension

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D42766 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D41102: Setup clang-doc frontend framework

JDevlieghere added inline comments. Comment at: tools/clang-doc/ClangDoc.h:40 + + void ParseUnattachedComments(); + bool ParseNewDecl(const NamedDecl *D); I know it's confusing given the amount of existing code that uses UpperCamelCase for functions, but I thi

[PATCH] D41102: Setup clang-doc frontend framework

JDevlieghere added a comment. I don't know what basis is used to differentiate between the two, but should this be part of clang tools or clang-tools-extra? https://reviews.llvm.org/D41102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D39834: [clang] -foptimization-record-file= should imply -fsave-optimization-record

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Thanks Dmitry, this LGTM! PS: Let me know if you don't have commit access and want me to commit it for you. https://reviews.llvm.org/D39834 ___

[PATCH] D39834: [clang] -foptimization-record-file= should imply -fsave-optimization-record

This revision was automatically updated to reflect the committed changes. Closed by commit rC321090: [clang] -foptimization-record-file= should imply -fsave-optimization-record (authored by JDevlieghere, committed by ). Repository: rC Clang https://reviews.llvm.org/D39834 Files: lib/Driver/

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere created this revision. JDevlieghere added reviewers: aprantl, dexonsmith, dblaikie, labath. As brought up during the discussion of the DWARF5 accelerator tables, there is currently no way to associate Objective-C methods with the interface they belong to, other than the .apple_objc ac

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere updated this revision to Diff 151580. JDevlieghere marked 5 inline comments as done. JDevlieghere added a comment. CR feedback https://reviews.llvm.org/D48241 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugInfo.h clang/test/CodeGenObjC/debug-info-synthesis.

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere added a comment. In https://reviews.llvm.org/D48241#1134240, @dblaikie wrote: > Not sure I should get too involved in this discussion, knowing so little > about Objective C - but if it seems sensible, could you provide some examples > (just in comments/description in this code revi

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere updated this revision to Diff 151591. JDevlieghere added a comment. - Use type cache & - Simplify method cache struct - Add custom test that verifies category methods are emitted https://reviews.llvm.org/D48241 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugI

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere updated this revision to Diff 151596. JDevlieghere marked 3 inline comments as done. JDevlieghere added a comment. - Feedback Adrian https://reviews.llvm.org/D48241 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugInfo.h clang/test/CodeGenObjC/debug-info-cate

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere added a comment. In https://reviews.llvm.org/D48241#1134985, @labath wrote: > This is not true. (Unlike the .apple_*** tables, ) .debug_names allows you to > specify a different schema for every entry in the acelerator table. The > schema is specifing using abbreviations in much th

[PATCH] D53200: [OpenCL] Fix serialization of OpenCLExtensionDecls

JDevlieghere added a comment. Can you add a test case please? Repository: rC Clang https://reviews.llvm.org/D53200 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53200: [OpenCL] Fix serialization of OpenCLExtensionDecls

JDevlieghere added a comment. The patch looks fine but since I don't know much about opencl I'll leave the LGTM to someone that actually knows this code. Comment at: test/Headers/opencl-pragma-extension-begin.h:1 + +#pragma OPENCL EXTENSION cl_my_ext : begin I

[PATCH] D57411: [ModuleDependencyCollector] Use llvm::sys::fs::real_path

JDevlieghere created this revision. JDevlieghere added a reviewer: bruno. Use the real_path from llvm::sys::fs::real_path instead of having a custom implementation. Repository: rC Clang https://reviews.llvm.org/D57411 Files: clang/lib/Frontend/ModuleDependencyCollector.cpp Index: clang/

[PATCH] D57411: [ModuleDependencyCollector] Use llvm::sys::fs::real_path

This revision was automatically updated to reflect the committed changes. Closed by commit rC352605: [ModuleDependencyCollector] Use llvm::sys::fs::real_path (NFC) (authored by JDevlieghere, committed by ). Changed prior to commit: https://reviews.llvm.org/D57411?vs=184155&id=184250#toc Reposi

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

JDevlieghere added a comment. The patch applies for me but has quite a few style violations. I'll fix those up before landing it. https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

This revision was automatically updated to reflect the committed changes. Closed by commit rL346601: Pass the function type instead of the return type to FunctionDecl::Create (authored by JDevlieghere, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://rev

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

JDevlieghere added a comment. In https://reviews.llvm.org/D53263#1294497, @kristina wrote: > In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote: > > > The patch applies for me but has quite a few style violations. I'll fix > > those up before landing it. > > > Thank you and sorry fo

[PATCH] D54310: Make clang-based tools find libc++ on MacOS

JDevlieghere added a comment. I reverted this because it broke the LLDB bots on GreenDragon: http://green.lab.llvm.org/green/view/LLDB/ Repository: rL LLVM https://reviews.llvm.org/D54310 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D55128: [CMake] Store path to vendor-specific headers in clang-headers target property

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. This makes sense to me. I'm don't know if there's a better property but I think this matches the intended use, so I think it is fine. Repository: rC Clang CHANGES SINCE LAST AC

[PATCH] D58941: [clang-format][docs][NFC] Fix example for Allman brace breaking style

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM. Good catch, left looks like K&R or something. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58941/new/ https://reviews.llvm.org/D58941 ___

[PATCH] D58930: Add XCOFF triple object format type for AIX

JDevlieghere requested changes to this revision. JDevlieghere added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1470 + case Triple::XCOFF: +// TODO: Falling through for XCOFF format for now. +break; --

[PATCH] D58930: Add XCOFF triple object format type for AIX

JDevlieghere added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2079 + if (log) +log->Printf("sorry: unimplemented for XCOFF"); + return false; jasonliu wrote: > JDevlieghere w

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

JDevlieghere added inline comments. Comment at: lib/AST/ASTImporter.cpp:1951 + // something we're trying to import while completin ToEnum + Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum); + ``` if (Decl *ToOrigin = Importer.GetOriginalDecl(ToEnum)) if (au

[PATCH] D60108: [os_log] Mark os_log_helper `nounwind`

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

[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration

JDevlieghere added a comment. I think this is change is causing an assertion to be hit: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/25103/consoleFull#-239233992d489585b-5106-414a-ac11-3ff90657619c http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/56122/console Can you ha

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

JDevlieghere added a comment. I've reverted this in rL360192 because it breaks stage 2 builds on GreenDragon. Please see the commit message and the inline comment for more details. Comment at: lib/Headers/CMakeLists.txt:36 bmiintrin.h +

[PATCH] D61399: [OpenMP][Clang] Support for target math functions

JDevlieghere added inline comments. Comment at: lib/Headers/CMakeLists.txt:36 bmiintrin.h + openmp_wrappers/math.h + openmp_wrappers/cmath hfinkel wrote: > JDevlieghere wrote: > > This doesn't do what you think it would do. The files are copied into the > >

[PATCH] D49549: Change 'clang-test' to 'check-clang' on the hacking webpage

JDevlieghere added a comment. Hey Arnaud, let me know if you want me to commit this for you. Repository: rC Clang https://reviews.llvm.org/D49549 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D52058: Add Parameters to DW_AT_name Attribute of Template Variables

JDevlieghere added a comment. Generally this looks good, but I'd like for the other to have a look first (at this and the other patch) before accepting. Comment at: lib/CodeGen/CGDebugInfo.cpp:1783 + if (auto *TS = dyn_cast(VL)) { +if (TS->getSpecializedTemplateOrPartial(

[PATCH] D52058: Add Parameters to DW_AT_name Attribute of Template Variables

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D52058 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D57976: -gmodules: Don't emit incomplete breadcrumbs pointing to nonexistant PCM files.

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. This makes sense to me, just one nit. Comment at: lib/CodeGen/CGDebugInfo.cpp:2304 +assert((!M || (M->Name == CGM.getLangOpts().ModuleName)) && + "c

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

JDevlieghere created this revision. JDevlieghere added reviewers: bruno, vsapsai. Herald added a project: clang. For reproducers in LLDB we want to hook up into the existing clang infrastructure. To make that happen we need to be able to override the ModuleDependencyCollector's methods. Reposi

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

JDevlieghere added a comment. In D58072#1393640 , @bruno wrote: > How much of the `ModuleDependencyCollector` will be reused as is by LLDB? I > wonder about the tradeoff versus inheriting from `DependencyCollector` > directly. We reuse the `attachTo` m

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

JDevlieghere added a comment. In D58072#1393817 , @bruno wrote: > Not really. Would making only the `attachTo*` methods virtual enough though? You mean making them **not** virtual? They're the only ones I don't override. Repository: rC Clang CHANGES

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

JDevlieghere added a comment. Alright, I'll split this up in two patches Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58072/new/ https://reviews.llvm.org/D58072 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D58072: Make ModuleDependencyCollector's method virtual (NFC)

This revision was automatically updated to reflect the committed changes. Closed by commit rL353882: Make ModuleDependencyCollector's method virtual (NFC) (authored by JDevlieghere, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: h

[PATCH] D37390: [diagtool] Add list-warning-flags

JDevlieghere created this revision. JDevlieghere added a project: clang. Herald added a subscriber: mgorny. This patch adds a new tool to diagnostic tool called `list-warning-flags` to display only warnings that have a corresponding -W flag. While we already have `list-warnings`, the output cont

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

JDevlieghere updated this revision to Diff 113731. JDevlieghere added a comment. Herald added subscribers: aheejin, klimek. Thanks for the review Adrian! I somehow completely overlooked the existing `--flags-only` option in `diagtool tree`. Obviously it makes much more sense to implement printin

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

JDevlieghere updated this revision to Diff 113737. JDevlieghere added a comment. Thanks Alex! https://reviews.llvm.org/D37390 Files: test/Misc/warning-flags-tree.c tools/diagtool/TreeView.cpp Index: tools/diagtool/TreeView.cpp ===

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

This revision was automatically updated to reflect the committed changes. Closed by commit rL312546: [diagtool] Change default tree behavior to print only flags (authored by JDevlieghere). Changed prior to commit: https://reviews.llvm.org/D37390?vs=113737&id=113885#toc Repository: rL LLVM h

[PATCH] D62654: [Docs] Modernize references to macOS

JDevlieghere accepted this revision. JDevlieghere 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/D62654/new/ https://reviews.llvm.org/D62654 ___

[PATCH] D65116: [Driver] Set the default win32-macho debug format to DWARF

JDevlieghere added inline comments. Comment at: clang/lib/Driver/ToolChains/MSVC.h:81 /// Set CodeView as the default debug info format. Users can use -gcodeview /// and -gdwarf to override the default. Let's update the comment. CHANGES SINCE LAST ACTI

[PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure

JDevlieghere added inline comments. Comment at: clang/include/clang/Basic/FileManager.h:217 /// - /// This returns NULL if the file doesn't exist. + /// This returns a \c std::error_code if there was an error loading the file. /// harlanhaskins wrote: > j

[PATCH] D65854: [diagtool] Use `operator<<(Colors)` to print out colored output.

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM with Fangrui's comment addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65854/new/ https://reviews.llvm.org/D65854 _

[PATCH] D65545: Handle some fs::remove failures

JDevlieghere added a comment. Thanks for looping me in, @vsapsai! Comment at: llvm/include/llvm/Support/FileUtilities.h:52-53 if (DeleteIt) { -// Ignore problems deleting the file. -sys::fs::remove(Filename); +if (std::error_code EC = sys::fs::rem

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; Is this really necessary? Comment at: clang/lib/Driver/Driver.cpp:980 + // We might tr

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Two small suggestions for the test, but this change LGTM. Comment at: clang/test/Driver/vfsmode.py:33 +new_name_len = target_len - len(absolute) - 1 +new_

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere added inline comments. Comment at: clang/lib/Driver/Driver.cpp:172 + case VFSMode::Unknown: +if (!this->VFS) { + LLVM_FALLTHROUGH; jfb wrote: > JDevlieghere wrote: > > Is this really necessary? > the `Driver` ctor takes a `VFS` parameter w

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere accepted this revision. JDevlieghere added a comment. Thanks for bearing with me, JF! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65986/new/ https://reviews.llvm.org/D65986 ___ cfe-commi

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

JDevlieghere added inline comments. Comment at: clang/include/clang/Driver/Options.td:2012 + HelpText<"Use the virtual file system in 'real' mode, or 'physical' mode. In 'real' mode the working directory is linked to the process' working directory. In 'physical' mode it has it

[PATCH] D66240: Remove LVALUE / RVALUE workarounds

JDevlieghere accepted this revision. JDevlieghere 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/D66240/new/ https://reviews.llvm.org/D66240 ___

[PATCH] D64526: [NFC] Unforget a colon in a few CHECK: directives.

JDevlieghere added a comment. The dsymutil part LGTM Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64526/new/ https://reviews.llvm.org/D64526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.

[PATCH] D67613: [DWARF-5] Support for DWARF-5 C++ language tags

JDevlieghere added a comment. I reverted this in r372672 because it caused a test failure in LLDB: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/1748/ Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67613/new/ https://reviews.llvm.org/D67613 _

[PATCH] D45255: [CodeGen] Add an option to suppress output of llvm.ident

JDevlieghere added inline comments. Comment at: test/CodeGen/no-ident-version.c:1 +// RUN: %clang_cc1 -Qn -emit-llvm -debug-info-kind=limited -o - %s | FileCheck %s + Please test both -Qy and -Qn as well as the default case. https://reviews.llvm.org/D45255

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

JDevlieghere added a comment. Please run your changes through clang format before checking this in; otherwise LGTM. Repository: rC Clang https://reviews.llvm.org/D45045 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.or

[PATCH] D45255: [CodeGen] Add an option to suppress output of llvm.ident

JDevlieghere added a comment. I’m happy with the test, thanks! In https://reviews.llvm.org/D45255#1072335, @aprantl wrote: > I'm not sure. Compatibility with GCC and getting identical output for > debugging purposes seem to be at odds here. I personally lean slightly > towards stripping it fro

[PATCH] D45255: [CodeGen] Add an option to suppress output of llvm.ident

JDevlieghere added a comment. In https://reviews.llvm.org/D45255#1073710, @miyuki wrote: > In https://reviews.llvm.org/D45255#1073703, @JDevlieghere wrote: > > > If it were the other way around I’d agree with you, but given that -Qn > > becomes the default, I think we should not strip it from th

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere added a comment. I tested this patch on the LLDB test suite and all tests passed. What I did was: - I removed the DWARF version check in clang so this was always generated. - I commented out the code that reads the .apple_objc accelerator tables in DWARFASTParserClang.cpp (which as

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere updated this revision to Diff 152883. JDevlieghere added a comment. - Update diff to version I used for testing (modulo the removed DWARF version check) https://reviews.llvm.org/D48241 Files: lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.h Index: lib/CodeGen/CGDebugInf

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere updated this revision to Diff 152896. JDevlieghere added a comment. - Re-add test case (forgot to stage it for last patch) https://reviews.llvm.org/D48241 Files: lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.h test/CodeGenObjC/debug-info-category.m Index: test/CodeGenO

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere updated this revision to Diff 152910. JDevlieghere marked 6 inline comments as done. JDevlieghere added a comment. - Feedback Adrian & Duncan https://reviews.llvm.org/D48241 Files: lib/CodeGen/CGDebugInfo.cpp lib/CodeGen/CGDebugInfo.h test/CodeGenObjC/debug-info-category.m I

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

JDevlieghere added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4239 +// Add methods to interface. +for (auto p : ObjCMethodCache) { + if (p.second.empty()) dexonsmith wrote: > Some comment for "p" here. Fixed; I'll update the rest of the

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

This revision was automatically updated to reflect the committed changes. Closed by commit rC335757: [DebugInfo] Emit ObjC methods as part of interface (authored by JDevlieghere, committed by ). Repository: rC Clang https://reviews.llvm.org/D48241 Files: lib/CodeGen/CGDebugInfo.cpp lib/Co

[PATCH] D69213: Avoid appending the source directory to an absolute path

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM modulo the comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69213/new/ https://reviews.llvm.org/D69213 ___ cfe-co

[PATCH] D69213: Avoid appending the source directory to an absolute path

JDevlieghere added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:541 +MainFileDir = MainFile->getDir()->getName(); +if (MainFileDir != "." && !llvm::sys::path::is_absolute(MainFileName)) { llvm::SmallString<1024> MainFileDirSS(MainFileDir); ---

[PATCH] D80770: [diagtool] Install diagtool when LLVM_INSTALL_TOOLCHAIN_ONLY is ON.

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Thanks, Volodymyr! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80770/new/ https://reviews.llvm.org/D80770 _

[PATCH] D84565: [Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

JDevlieghere requested changes to this revision. JDevlieghere added a comment. This revision now requires changes to proceed. When you don’t pass any specific options to the linker, it’s going to generate a temporary `lto.o` file in `/tmp` and delete it after the link. When `dsymutil` will try t

[PATCH] D84572: Allow .dSYM's to be directly placed in an alternate directory

JDevlieghere added inline comments. Comment at: clang/include/clang/Driver/Options.td:693 def e : JoinedOrSeparate<["-"], "e">, Group; +def external_dsym_dir : JoinedOrSeparate<["-"], "external-dsym-dir">, + Flags<[DriverOption, RenderAsInput]>, Could we drop t

[PATCH] D84572: Allow .dSYM's to be directly placed in an alternate directory

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84572/new/ https://reviews.llvm.org/D84572 __

[PATCH] D83454: [CMake] Make `intrinsics_gen` dependency unconditional.

JDevlieghere added a comment. LGTM. I verified this works for the build-tree standalone build of LLDB, but please still keep an eye on http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/ after you land this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D83454: [CMake] Make `intrinsics_gen` dependency unconditional.

JDevlieghere added a comment. In D83454#2142719 , @michele.scandale wrote: > I tested locally the standalone build of Clang with D83426 > . > I just want to make sure that we all agree this is right way moving forward.

[PATCH] D82733: [clang][docs] Add note about using `-flto` with `-g` on macOS

JDevlieghere added inline comments. Comment at: clang/docs/CommandGuide/clang.rst:477 + Note: on Darwin, when using :option:`-flto` along with :option:`-g` and + compiling and linking in separate steps, you also need to pass Any reason not to use the sphinx n

[PATCH] D82733: [clang][docs] Add note about using `-flto` with `-g` on macOS

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM. Thank you for adding this! In D82733#2121414 , @phil-blain wrote: > I tested the sphinx build for the `man` and `html` targets. Any o

  1   2   3   >