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

kimgr added a comment. From a purely personal perspective, I'd prefer if this landed after the branch for llvm-15. We try to co-release IWYU shortly after LLVM/Clang are released, to get a public API-compatible release out there. So it would be really nice if we didn't have to spend time worki

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

kimgr added a comment. In D112374#3657640 , @mizvekov wrote: > In D112374#3657472 , @kimgr wrote: > >> I'm coming at this from pretty far away, so there's very likely lots of >> details that I'm overlooking. But

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

kimgr added a comment. > It's the difference in knowing the type was written without any tag or > nested-name specifier, and having a type that you are not sure how it was > written. > > When we are dealing with a type which we are not sure, we would like to print > it fully qualified, with a s

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

kimgr added a comment. It's a little confusing, because it now looks like _every_ `Type` in the AST is wrapped in an `ElaboratedTypeLoc` + `ElaboratedType`. IWYU's debug AST dump shows this (excerpt): tests/cxx/sizeof_reference.cc:51:8: (1) [ VarDecl ] size_t s2

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

kimgr added a comment. This patch also broke IWYU, not exactly sure how or why yet. We run around the AST quite a bit, so structural changes like this often bite us. Can you expand on what happened here? Before/after kind-of thing? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr closed this revision. kimgr added a comment. Herald added a project: All. Now that D119477 has landed, this suggested change is obsolete. Closing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ h

[PATCH] D119476: Generalize and harmonize sub-expression traversal

kimgr added a comment. Thanks! Could you help me land it? Author: Kim Gräsman . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119476/new/ https://reviews.llvm.org/D119476 ___ cfe-commits mailing list cfe

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

kimgr updated this revision to Diff 416695. kimgr added a comment. Address review feedback - Restore accidentally removed test line - Clarify diagnostic expectations Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119477/new/ https://reviews.llvm.or

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

kimgr added inline comments. Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:362-364 + { A k = to_lvalue_ref(A().ret_a()); } // expected-error {{'alloc::A::ret_a' is not a constant expression}} expected-error {{'alloc::to_lvalue_ref' is not a constant expression}} + // exp

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

kimgr added a comment. @aaron.ballman Friendly Monday ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119477/new/ https://reviews.llvm.org/D119477 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D119476: Generalize and harmonize sub-expression traversal

kimgr added a comment. @aaron.ballman Friendly Monday ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119476/new/ https://reviews.llvm.org/D119476 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

kimgr added a comment. Herald added a project: All. Gentle weekly ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119477/new/ https://reviews.llvm.org/D119477 ___ cfe-commits mailing list cfe-commits@

[PATCH] D119476: Generalize and harmonize sub-expression traversal

kimgr added a comment. Herald added a project: All. Gentle weekly ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119476/new/ https://reviews.llvm.org/D119476 ___ cfe-commits mailing list cfe-commits@

[PATCH] D119476: Generalize and harmonize sub-expression traversal

kimgr updated this revision to Diff 411666. kimgr added a comment. Fix typo in comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119476/new/ https://reviews.llvm.org/D119476 Files: clang/lib/AST/Expr.cpp Index: clang/lib/AST/Expr.cpp

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

kimgr added a comment. I have a local branch with both D119476 and this patch. - Rebased on latest main (853ca5472314e109b98e46f0985f27f79e17d2bd ) - Ran `ninja check-clang` and `ninja tools/

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

kimgr added a comment. Having looked at this some more, I wonder if this patch is covering for another problem. Running even the simplest example fails on an assertion in `Sema::BuildCXXTypeConstructExpr`: https://godbolt.org/z/fMPcsh4f3. It looks like that function is creating the majority of

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

kimgr added a comment. I can also confirm that `main` and my branch (containing https://reviews.llvm.org/D119477 and https://reviews.llvm.org/D119476) both assert on the same line: $ cat bug-53244.cc struct A { consteval A() {} A(const A&) = delete; consteval void

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

kimgr added a comment. First sanity check: applying this patch on my branch causes no test failures either in the `tools/clang/unittests/Tooling/ToolingTests` or `ninja check-clang-semacxx`. Hopefully I can think this through more deeply soon. Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

kimgr added a comment. Thanks for the heads-up! I'm a little busy this week, but I need to think about potential interference between these two changes. Off the top of my head it looks like they should get along fine, but there may be subtleties. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D119476: Generalize and harmonize sub-expression traversal

kimgr added inline comments. Comment at: clang/lib/AST/Expr.cpp:1903 +// Skip over implicit nodes produced as part of semantic analysis. +// Designed for use with IgnoreExpreNodes. +Expr *ignoreImplicitSemaNodes(Expr *E) { Typo: IgnoreExpr*e*Nodes Repository:

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

kimgr added a subscriber: aaron.ballman. kimgr added a comment. @aaron.ballman This patch replaces https://reviews.llvm.org/D117391 as a potential solution for https://github.com/llvm/llvm-project/issues/53044. I can't swear that the new diagnostics in cxx2a-consteval.cpp are standards-conformi

[PATCH] D119477: Ignore FullExpr when traversing cast sub-expressions

kimgr created this revision. kimgr requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Full-expressions are Sema-generated implicit nodes that cover constant-expressions and expressions-with-cleanup for temporaries. Ignore those as part of imp

[PATCH] D119476: Generalize and harmonize sub-expression traversal

kimgr added a subscriber: aaron.ballman. kimgr added a comment. @aaron.ballman First refactor to generalize and harmonize `getSubExprAsWritten` and `getConversionFunction`. As mentioned in the commit message, this is strictly speaking a functional change, but it should have no visible effect.

[PATCH] D119476: Generalize and harmonize sub-expression traversal

kimgr created this revision. Herald added a subscriber: kristof.beyls. kimgr requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. CastExpr::getSubExprAsWritten and getConversionFunction used to have disparate implementations to traverse the sub-

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @aaron.ballman Thanks for the valuable feedback! > Assuming that the `to_lvalue_ref(A{})` case should diagnose, then yes, I > agree, I think the `ret_a()` portion should as well. My primary focus is not to make sure Clang follows the standard closely for `consteval`, but

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. I have now convinced myself that including `FullExpr` in `skipImplicitTemporary` gives an improvement in `consteval` diagnostics. But I'm still not sure why. Motivating example, derived from cxx2a-consteval.cpp: struct A { int *p = new int(42); consteval A ret_a

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added inline comments. Comment at: clang/lib/AST/Expr.cpp:1949-1950 if (E->getCastKind() == CK_ConstructorConversion) return cast(SubExpr)->getConstructor(); kimgr wrote: > davrec wrote: > > I think the errors prove we should fall back to th

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added inline comments. Comment at: clang/lib/AST/Expr.cpp:1949-1950 if (E->getCastKind() == CK_ConstructorConversion) return cast(SubExpr)->getConstructor(); davrec wrote: > I think the errors prove we should fall back to the most conservati

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. Here's the diff between my patch (main) and handling `FullExpr` in `skipImplicitTemporary` as in the diff I posted above (patch.txt): --- main.txt 2022-02-02 20:37:21.619534225 +0100 +++ patch.txt 2022-02-02 20:34:17.016949227 +0100 @@ -192,6 +192,13 @@ /home/kim

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. Alright, with this diff: diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index d502b3f1e93e..1716ac0be7ef 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -1908,6 +1908,9 @@ namespace { if (auto *Binder = dyn_cast(E))

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @aaron.ballman @davrec Thank you both! I started with @davrec's suggestion from https://github.com/llvm/llvm-project/issues/53044#issuecomment-1006894536, and while it fixed the problem it broke several other consteval lit tests. It could be that those test breaks are be

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

kimgr added a comment. @aaron.ballman Thanks! Please use Kim Gräsman and kim.grasman at gmail.com. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117390/new/ https://reviews.llvm.org/D117390 ___ cfe-commi

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @rsmith Gentle ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

kimgr added a comment. @rsmith Gentle ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117390/new/ https://reviews.llvm.org/D117390 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

kimgr added a comment. We've had reports from users that it's now possible to build IWYU together with Clang and LLVM using LLVM_EXTERNAL_PROJECTS, so this can be closed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D31696/new/ https://reviews.llvm.org/D31696 _

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @rsmith Weekly ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

kimgr added a comment. @rsmith Weekly ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117390/new/ https://reviews.llvm.org/D117390 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. - Update broken test case - Rebase on latest main (8eb74626f ) - Build and run `ninja check-clang` I don't have commit access, so I would appreciate help landing. Let me know if there's anything else I c

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr updated this revision to Diff 400381. kimgr added a comment. Fix spurious semicolon Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391 Files: clang/lib/AST/Expr.cpp clang/unittests/Tooling/CastExpr

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added inline comments. Comment at: clang/unittests/Tooling/CastExprTest.cpp:111 + "const char *f() {\n" + " constexpr X x;\n"; + " return x;\n" Oops, spurious semicolon here. Will post a new version once

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

kimgr added a comment. @daverec I don't have commit access, could you help me land this? I've rebased on `main` without conflicts, and `ninja check-clang` ran successfully after rebase. Let me know if there's anything else I can do as far as prep work. Repository: rG LLVM Github Monorepo C

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. Oh yeah, this closes bug https://github.com/llvm/llvm-project/issues/53044. Do I mention that in the commit message/patch summary? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391 _

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr created this revision. kimgr requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Under -std=c++20 with use of consteval, the subexpression hierarchy sometimes contains ConstantExpr nodes that getConversionFunction did not expect. CastExp

[PATCH] D117390: [AST] Reformat CastExpr unittest suite

kimgr created this revision. kimgr requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. In preparation for adding new tests. No functional change. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D117390 Files: clang/unittest

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

kimgr added a comment. @dblaikie I think @sammccall mentioned `IWYU pragma: friend` before, that's what I was alluding to. Much of IWYU's complexity comes from maintaining these library mappings both statically (using mapping tables) and dynamically (using in-source annotations). I figured the

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

kimgr added a comment. Hi, sorry I'm late to the game. IWYU maintainer here. I saw this patch mentioned in the LLVM Weekly newsletter and immediately thought: "wow, great, I have to build support in IWYU for that!". I too prefer pragmas to magic comments, and I don't think `include_instead` ne

[PATCH] D54047: Check TUScope is valid before use

kimgr added a comment. This was eventually fixed in IWYU based on @jkorous' suggestion above. I believe the problem is/was: - After code is parsed and the AST is built, Sema resets its `TUScope` member to null - We use Sema to lookup and define default constructors before traversing the AST us

[PATCH] D31697: Check for null before using TUScope

kimgr added a comment. This was eventually fixed in IWYU. I believe the problem is/was: - After code is parsed and the AST is built, Sema resets its `TUScope` member to null - We use Sema to lookup and define default constructors before traversing the AST using a RAV - Some yet-not-fully-identi

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

kimgr added a comment. In D72703#1834177 , @hans wrote: > I assume the same correlation could also be found with lines of code, but I > think tokens is a better dimension to measure since it's less likely to be > gamed, and it also is also kind of the ba

[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

kimgr added a comment. A small Python suggestion. Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:112 + + if "Notes" in diag: +for note in diag["Notes"]: This double lookup is unnecessary, you can do `for note in diag.get("Notes", []):` to

[PATCH] D72703: Add a warning, flags and pragmas to limit the number of pre-processor tokens in a translation unit

kimgr added a comment. I just want to say that finding the correlation between token count and compile time is a bit of a breakthrough! Could you expose a flag for printing token count so users can run their own analysis? Or does that already exist in baseline clang? It's easier to set a maximu

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

kimgr added a comment. Oh, I see. I thought `rc.exe` was the one and only implementation. Fair enough, this is probably the lesser of two evils :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70196/new/ https://reviews.llvm.org/D70196

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

kimgr added a comment. I wonder if it would be better to look at the compile-command than the source file? It's not unthinkable that someone would use another extension for an rc file, or use a .rc extension for a C or C++ source file. But if the compiler is rc.exe, the source file must be in

[PATCH] D68163: [analyzer][MallocChecker][NFC] Change the use of IdentifierInfo* to CallDescription

kimgr added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1006 const FunctionDecl *FD = C.getCalleeDecl(CE); - if (!FD) + if (!FD || FD->getKind() != Decl::Function) return; NoQ wrote: > The `FD->getKind() != Decl::Fun

[PATCH] D61909: Add Clang shared library with C++ exports

kimgr added a comment. @sylvestre.ledru @beanz After this change, the Debian packaging on apt.llvm.org is basically broken. See https://bugs.llvm.org/show_bug.cgi?id=42432. I'm sure this can be fixed in packaging, but I don't know enough about it to know exactly what to do. At any rate I thin

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

kimgr added a comment. In D62115#1547705 , @skan wrote: > In D62115#1547698 , @kimgr wrote: > > > This looks good to me, thanks! > > > Could you help accept this patch? I'm not active in the project, so I don't fe

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

kimgr added a comment. This looks good to me, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62115/new/ https://reviews.llvm.org/D62115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

kimgr added a comment. I think the test needs a bit more work. It's essentially checking the same thing twice to exercise the Windows path separators. It looks like there's already a test for `-H` in `FrontEnd/print-header-includes.c`. That also demonstrates the use of `--check-prefix` to hand

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

kimgr added a comment. Also, consider `././Inputs/empty.h`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62115/new/ https://reviews.llvm.org/D62115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mail

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

kimgr added a comment. I was thinking about a testcase like: // RUN: %clang -H -fsyntax-only %s 2>&1 | FileCheck %s #include "..\Index\Inputs\empty.h" #include ".\Inputs\empty.h" // CHECK: . // CHECK-SAME: ??? // CHECK: . // CHECK-NOT: ??? // CHECK-SAME: ??? with some provi

[PATCH] D62115: fix a issue that clang is incompatible with gcc with -H option.

kimgr added a comment. Should you also consider Windows path separators here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62115/new/ https://reviews.llvm.org/D62115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llv

[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

kimgr added inline comments. Comment at: clang/include/clang/Tooling/FixIt.h:73 +// context. In contrast with \p getText(), this function selects a source range +// "automatically", extracting text that a reader might intuitively associate +// with a node. Currently, only specia

[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

kimgr added inline comments. Comment at: clang/include/clang/Tooling/FixIt.h:73 +// context. In contrast with \p getText(), this function selects a source range +// "automatically", extracting text that a reader might intuitively associate +// with a node. Currently, only specia

[PATCH] D54047: Check TUScope is valid before use

kimgr added subscribers: zturner, kimgr. kimgr added a comment. Here's some related suggestions/questions for context: - Earlier patch from @zturner with a minimal repro case: https://reviews.llvm.org/D31697. I don't think this is reproducible with Clang proper. - Open question for alternative

[PATCH] D49922: [P0936R0] add [[clang::lifetimebound]] attribute

kimgr added a comment. Potential typo in the tests Comment at: test/SemaCXX/attr-lifetimebound.cpp:24 + + // Do not diagnose non-void return types; they can still be lifetime-bound. + long long ptrintcast(int ¶m [[clang::lifetimebound]]) { Should this say 'no

[PATCH] D49486: [cfe][CMake] Export the clang resource directory

kimgr added a comment. Thank you for doing this, I'm going to guess you have IWYU in mind :-) So as consumers of this, how do you envision we use the new variable? Something like https://stackoverflow.com/a/13429998 to copy the resource dir into our build root, and then an install rule to put i

[PATCH] D32696: More detailed docs for UsingShadowDecl

kimgr added a comment. Herald added a subscriber: llvm-commits. Ping! Repository: rL LLVM https://reviews.llvm.org/D32696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

kimgr added a comment. Herald added a reviewer: javed.absar. This can be closed, IWYU no longer officially supports in-tree builds. https://reviews.llvm.org/D31696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

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

kimgr added a comment. >> Can you test what happens when you do clang-cl.exe /Yc stdafx.h /Tp >> stdafx.h, i.e. compile the header > > directly as C++ code and generate .pch from it? The normal MSVC modus > operandi is to have stdafx.h > > included in all source files (with /Yu) and stdafx.c

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

kimgr added a comment. I've done some terrible things with MSVC PCHs over the years, so I'll pile on some more questions: - Can you test what happens when you do `clang-cl.exe /Yc stdafx.h /Tp stdafx.h`, i.e. compile the header directly as C++ code and generate .pch from it? The normal MSVC m

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

kimgr added a comment. It works! Having to specify `CMAKE_PREFIX_PATH` is at least documentable, so I don't see that as a major drawback, especially if it makes docs for tools using Clang more portable. Thank you for moving this along, now I can vastly simplify our CMake build. Repository:

[PATCH] D46522: [clang] cmake: resolve symlinks in ClangConfig.cmake

kimgr added a comment. I'm interested in this, I've tried for a while to fix the Debian packaging but I'm completely new to the packaging toolchain, so I'm making slow headway. Some of the problems are recorded in: https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=862328 My (possibly nai

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

kimgr added a comment. > When using a PCH, tokens are skipped until after the through header is seen. I know this is what MSVC does (though in their case I think it's an artifact of their implementation), but I remember this being a source of mysterious bugs: #define MY_SYMBOL 1 #include "s

[PATCH] D43578: -ftime-report switch support in Clang

kimgr added a comment. In https://reviews.llvm.org/D43578#1067974, @rsmith wrote: > In https://reviews.llvm.org/D43578#1067891, @kimgr wrote: > > > I disagreed up until the last paragraph :) > > > Can you say which things you disagree with? There seem to be two important > facts here: > > 1. LLV

[PATCH] D43578: -ftime-report switch support in Clang

kimgr added a comment. Heh, my e-mail response was eaten along the way. Continued here: On Sat, Apr 14, 2018 at 1:53 AM, Richard Smith - zygoloid via Phabricator via cfe-commits wrote: > rsmith added a comment. > > So I don't think this patch is reasonable for that reason. I'm also not sure >

[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.

kimgr added subscribers: lebedev.ri, kimgr. kimgr added a comment. I have to say I disagree that either the nested struct/function or macros (in any form) should count toward a function's total variable count. Both are valid forms of abstraction, and they both remove complexity from the containin

[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions

kimgr added a comment. Peanut gallery observation: there was a discussion on the Boost list years and years ago where someone made the case that `if (x != nullptr) delete x;` was measurably faster than just calling `delete x;` I can't find it now, but I think it might have been in the context o

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

kimgr added a comment. Herald added a subscriber: kristof.beyls. Ping! It would be nice to get some traction on this. https://reviews.llvm.org/D31696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listi

[PATCH] D41809: Clang counterpart change for buzzer FreeBSD support

kimgr requested changes to this revision. kimgr added a comment. This revision now requires changes to proceed. Typo in the commit title: buzzer :) Repository: rC Clang https://reviews.llvm.org/D41809 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D39360: [C++11] Don't put empty quotes in static_assert diagnostic.

kimgr added a comment. Why? It seems easier to me to map back if the diagnostic mirrors the code as-written. https://reviews.llvm.org/D39360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D36156: [rename] Introduce symbol occurrences

kimgr added a comment. I was about to suggest SymbolLocation or even SymbolLoc, but it appears to keep track of more than locations. "Reference" sounds a little open-ended. No more ideas here, I'm afraid. Repository: rL LLVM https://reviews.llvm.org/D36156 ___

[PATCH] D36156: [rename] Introduce symbol occurrences

kimgr added a comment. High-level comment from the peanut gallery: The word "occurrences" is horrible to type and most people misspell it (you did great here!) Would you consider something like "SymbolMatches" or even "SymbolHits"? Repository: rL LLVM https://reviews.llvm.org/D36156 ___

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

kimgr added a comment. Any chance you could try my example too? It seems like this approach might catch it. But does the new approach really do anything different for your original example? https://reviews.llvm.org/D35783 ___ cfe-commits mailing

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

kimgr added a comment. I've seen this in the wild: #define LOG(m) \ { \ std::ostringstream os; \ os << m << "\n"; \ LogWrite(os.str()); \ } auto os = GetOSName(); LOG("The OS is " << os); It looks like your patch would miss this case. Not sure if

[PATCH] D31697: Check for null before using TUScope

kimgr added a comment. > only for the function templates that use Microsoft intrinsics (e.g. > _BitScanForward in TrailingZerosCounter.) > So there's something in the parsing of builtins/intrinsics that requires > TUScope to be non-null. For posterity, this was misdiagnosed on my part. It turn

[PATCH] D31697: Check for null before using TUScope

kimgr added a comment. I did some more debugging today. This happens when we attempt to analyze llvm/Support/MathExtras.h, and only for the function templates that use Microsoft intrinsics (e.g. `_BitScanForward` in `TrailingZerosCounter`.) So there's something in the parsing of builtins/intrin

[PATCH] D31697: Check for null before using TUScope

kimgr added a comment. We'd love to see this addressed, either in our code or in Clang. But we're not sure what to do on our end, so... a gentle ping for help! https://reviews.llvm.org/D31697 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32480: [clang-format] Add BinPackNamespaces option

kimgr added a comment. We have a large closed-source codebase with this style, and would welcome clang-format support. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/lis

[PATCH] D32696: More detailed docs for UsingShadowDecl

kimgr created this revision. I couldn't make sense of the docs before, so I sent a question to cfe-dev. Richard was gracious enough to fill in the blanks: http://lists.llvm.org/pipermail/cfe-dev/2017-April/053687.html I've tried to transfer some of his response to the Doxygen for UsingShadowDecl

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

kimgr added a comment. Python 3 concern inline Comment at: clang-tidy/tool/run-clang-tidy.py:100 + except: +print >>sys.stderr, "Unable to run clang-apply-replacements." +sys.exit(1) alexfh wrote: > "Unable to run clang-apply-replacements" without any

[PATCH] D31697: Check for null before using TUScope

kimgr added a comment. For some context, the backtrace when this happens is: include-what-you-use.exe!clang::Scope::getEntity() Line 313C++ include-what-you-use.exe!clang::Sema::PushOnScopeChains(clang::NamedDecl * D=0x07ed05b0, clang::Scope * S=0x, bool AddToContex

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

kimgr added inline comments. Comment at: clang/tools/CMakeLists.txt:35 +# if include-what-you-use is cloned for building in-tree, add it here. +if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/include-what-you-use") + add_clang_subdirectory(include-what-you-use) beanz wr

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

kimgr added a comment. > BTW, kimgr@, is there any particular reason you haven't tried to upstream the > tool? > Maybe even as not a standalone tool but as a set of checks in clang-tidy. Lack of time, mostly. IWYU was written before there was libtooling, and follows Google style, so we've felt

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

kimgr added a comment. As an include-what-you-use maintainer, I would very much welcome this. I've been too shy to suggest it myself ;-). https://reviews.llvm.org/D31696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

kimgr added a comment. A few more 'inhertiance' left, otherwise spelling looks good to me :-) Comment at: lib/Format/TokenAnnotator.cpp:679 Tok->Type = TT_CtorInitializerComma; + else if (Contexts.back().InInhertianceList) +Tok->Type = TT_InheritanceComma;

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

kimgr added a comment. Oops, the flag is called `BreakBeforeInhertianceComma` with a misspelling throughout. May want to search for `inhertiance` throughout and see if there are more cases. Comment at: docs/ClangFormatStyleOptions.rst:531 +**BreakBeforeInhertianceComma** (`

[PATCH] D30773: Make git-clang-format python 3 compatible

kimgr added inline comments. Comment at: tools/clang-format/git-clang-format:323 allowed_extensions = frozenset(allowed_extensions) - for filename in dictionary.keys(): + for filename in list(dictionary.keys()): base_ext = filename.rsplit('.', 1) EricWF

[PATCH] D30773: Make git-clang-format python 3 compatible

kimgr added a comment. Some nits from a Python3 hacker. Comment at: tools/clang-format/git-clang-format:143 for filename in ignored_files: -print ' ', filename +print(' ', filename) if changed_lines: I find `print('template', tai

[PATCH] D30532: Add examples to clang-format configuration

kimgr added a comment. For what it's worth, I also find this useful to be able to see at a glance what the setting does. I know I've tried several different settings in the past before finding the one I was looking for. https://reviews.llvm.org/D30532 ___

[PATCH] D30564: [clang-tidy] Format code around applied fixes

kimgr added inline comments. Comment at: docs/clang-tidy/index.rst:184 -p= - Build path +-quiet - + Run clang-tidy in quiet mode. This suppresses This looks like a separate patch?

  1   2   >