[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall updated this revision to Diff 508272. sammccall added a comment. Move data into JSON object, render into DOM with templates. This is cleaner in a few ways: less ugly C++ string manipulation, data is in principle reusable, HTML layout is extracted and easier to inspect and edit.

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall planned changes to this revision. sammccall added a comment. Going to have a go at passing data via JSON Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146591/new/ https://reviews.llvm.org/D146591

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall updated this revision to Diff 507718. sammccall marked 2 inline comments as done. sammccall added a comment. Address all comments except moving data from HTML => JSON Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146591/new/

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall marked 15 inline comments as done. sammccall added inline comments. Comment at: clang/lib/Analysis/FlowSensitive/CMakeLists.txt:22 +add_custom_command(OUTPUT HTMLLogger.inc + COMMAND "${Python3_EXECUTABLE}" bundle_resources.py +

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

This revision was automatically updated to reflect the committed changes. Closed by commit rG48f97e575137: [FlowSensitive] Log analysis progress for debugging purposes (authored by sammccall). Changed prior to commit: https://reviews.llvm.org/D144730?vs=507418=507678#toc Repository: rG LLVM

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

sammccall added inline comments. Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:186 + Logger () const { +return DACtx->getOptions().Log ? *DACtx->getOptions().Log : Logger::null(); + } xazax.hun wrote: > If we already have a

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

sammccall updated this revision to Diff 507418. sammccall marked 3 inline comments as done. sammccall added a comment. address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144730/new/ https://reviews.llvm.org/D144730 Files:

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall added a comment. In D146591#4213319 , @xazax.hun wrote: > While I am OK with this approach, I wonder if it would be too much work to > split the HTML generation and emitting data up. E.g., the logger could emit > YAML or JSON that could be

[PATCH] D146625: [dataflow] handle missing case in value debug strings

sammccall created this revision. Herald added subscribers: martong, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All. sammccall published this revision for review. sammccall added a reviewer: gribozavr2. Herald added a project: clang. Herald added a subscriber: cfe-commits.

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall updated this revision to Diff 507339. sammccall added a comment. Use a process-shared counter for HTML output filenames to avoid clobbering. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146591/new/ https://reviews.llvm.org/D146591

[PATCH] D146527: [dataflow] Log flow condition to the correct stream.

This revision was automatically updated to reflect the committed changes. Closed by commit rGee2cd606abd9: [dataflow] Log flow condition to the correct stream. (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146527/new/

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall added a comment. Added a bunch of reviewers for feedback on how this should work. Happy to have detailed comments on the JS/HTML generator implementation too of course, but it may inevitably be a bit of a mess (ugly, weird languages, undertested). I've added things like this to a few

[PATCH] D146591: [dataflow] add HTML logger: browse code/cfg/analysis timeline/state

sammccall created this revision. Herald added subscribers: martong, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. With -dataflow-log=/dir we will write

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

sammccall added a comment. In D144730#4211222 , @NoQ wrote: > I love such debugging facilities! They're a massive boost to developer > productivity compared to ad-hoc debug prints, and they allow new contributors > to study how the tool works. I'm

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

sammccall added a reviewer: xazax.hun. sammccall added a comment. Herald added a subscriber: rnkovacs. Thanks! I cleaned up a bit and added tests, as well as a `-dataflow-log` flag to make this easy to access when running unit tests, tidy checks etc. The HTML experiments seem to have validated

[PATCH] D144730: [FlowSensitive] Log analysis progress for debugging purposes

sammccall updated this revision to Diff 507067. sammccall marked 2 inline comments as done. sammccall retitled this revision from "[FlowSensitive][WIP] log analysis progress for debugging purposes" to "[FlowSensitive] Log analysis progress for debugging purposes". sammccall edited the summary of

[PATCH] D145843: [clangd] Add option to always insert headers with <> instead of ""

sammccall added a comment. In D145843#4207466 , @kadircet wrote: > In D145843#4207101 , @nridge wrote: > >> My understanding is that a more elaborate configuration scheme has been >> proposed in

[PATCH] D146527: [dataflow] Log flow condition to the correct stream.

sammccall created this revision. sammccall added a reviewer: mboehme. Herald added subscribers: martong, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.

[PATCH] D145921: [clangd] Add missing unittests to build graph

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. doh, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145921/new/ https://reviews.llvm.org/D145921

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

sammccall added a comment. Sorry for the delay, extracting the repro from the build system seems about as much work as minimizing it :-) F26749550: modrepo2.zip - running repro.sh hits an assert for me on the main.cpp compile. (I suspect something much

[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

sammccall added a comment. We're seeing new clang crashes that bisect to this commit, with modules only. I have it mostly-reduced and will post shortly, trying to see if I can simplify any further (since modules reproducers are a pain). Meanwhile, the assert/stack in case it's already useful:

[PATCH] D145228: [clangd] Add clangd headers to install targets

sammccall added a comment. @aaron.ballman If someone wants to work on producing a more modular, component-based design, that's probably a good idea (but tricky to get right). Probably a good place to start is working out what pieces can be moved into separate libraries with e.g. a coherent

[PATCH] D145244: [clang-format] Add ability to trace tokens.

sammccall added a comment. Oops, sorry for the noise. I'd forgotten that we're really interested in *why* we reach the conclusion, not the conclusion itself - and that file/line are our proxy for "why" as writing/maintaining descriptions is a burden. Comment at:

[PATCH] D145228: [clangd] Add clangd headers to install targets

sammccall added a comment. In D145228#4170826 , @DmitryPolukhin wrote: > In D145228#4170792 , @sammccall > wrote: > >>> The install target for clang distributes the clangd static libs >> >> I don't think this

[PATCH] D145244: [clang-format] Add ability to trace tokens.

sammccall added inline comments. Comment at: clang/lib/Format/ContinuationIndenter.cpp:289 Current.closesBlockOrBlockTypeList(Style))) { +DEBUG_FORMAT_TRACE_TOKEN(Current, "!canBreak"); return false; annotating all

[PATCH] D145302: [clangd] Add library for clangd main function

sammccall added a comment. IIUC we want to link some extra targets into the `clangd` binary in order to provide extra clang-tidy checks, but *don't* want to customize the binary further. (The latter can be useful, but this interface isn't sufficient for it). Can this be solved at the

[PATCH] D145228: [clangd] Add clangd headers to install targets

sammccall added a comment. > The install target for clang distributes the clangd static libs I don't think this was ever intended, looks like an accidental side-effect of using LLVM's many cmake macros. Can we fix this instead? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D140794: [ASTMatcher] Add coroutineBodyStmt matcher

sammccall added a comment. It seems pretty weird to me that the two child edges from functiondecl -> coroutinebodystmt -> compoundstmt are both called "body". However that *is* what they're called in the AST today, and having the matchers correspond to the AST's names seems important. So this

[PATCH] D144730: [FlowSensitive][WIP] log analysis progress for debugging purposes

sammccall added a comment. I wanted to share this to get some initial thoughts as we'd discussed debuggability a while ago. As mentioned I'd at least want to get a reasonable HTML report before moving forward with it though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D144170: [clang-format] Add simple macro replacements in formatting.

sammccall added a comment. Still LG Comment at: clang/lib/Format/MacroExpander.cpp:172 assert(defined(ID->TokenText)); + assert( + (!OptionalArgs && ObjectLike.find(ID->TokenText) != ObjectLike.end()) || klimek wrote: > sammccall wrote: > > this

[PATCH] D144730: [FlowSensitive][WIP] log analysis progress for debugging purposes

sammccall updated this revision to Diff 500172. sammccall added a comment. Fix colors, log source Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144730/new/ https://reviews.llvm.org/D144730 Files: clang/examples/CMakeLists.txt

[PATCH] D144730: [FlowSensitive][WIP] log analysis progress for debugging purposes

sammccall added a comment. Example textual log (though better with colors): === Beginning data flow analysis === int target(int x) { while (x > 0) { --x; } return x + 1; } FunctionDecl 0x2a9a300 line:1:5 target 'int (int)' |-ParmVarDecl

[PATCH] D144730: [FlowSensitive][WIP] log analysis progress for debugging purposes

sammccall updated this revision to Diff 500166. sammccall added a comment. A little more docs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144730/new/ https://reviews.llvm.org/D144730 Files: clang/examples/CMakeLists.txt

[PATCH] D144730: [FlowSensitive][WIP] log analysis progress for debugging purposes

sammccall created this revision. Herald added a subscriber: wenlei. Herald added a reviewer: NoQ. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is just a draft and a starting point, and needs

[PATCH] D144170: [clang-format] Add simple macro replacements in formatting.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Great! The overloading impl is a little surprising to me. I was assuming that object would always win over function (or that it would be disallowed to combine the two). I think that is

[PATCH] D144170: [clang-format] Add simple macro replacements in formatting.

sammccall added a comment. Thanks, this all looks good, at least as far as I understand it! The two-passes-over-the-whole-file approach is easier for me to follow, too. Mostly just comment nits, but I have one annoying question left about using macros with the wrong arity - I thought this was

[PATCH] D144054: [Lex] Fix a crash in updateConsecutiveMacroArgTokens.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Lex/TokenLexer.cpp:1029 + // Including Limit will not mischeck for across-file-id tokens + // because SourceManager allocates

[PATCH] D144456: [clangd] Publish diagnostics with stale preambles

sammccall added a comment. Looks pretty good. I think we can build the main file & preamble in parallel. Otherwise nits. Comment at: clang-tools-extra/clangd/TUScheduler.cpp:946 +// gurantee eventual consistency. +if (LatestPreamble &&

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG thanks! Comment at: clang-tools-extra/clangd/Preamble.cpp:496 +auto CurrentStartIt = CurrentContentsToLine.find(OldLines[OldStart]); +if (CurrentStartIt ==

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

sammccall added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:625 +// same spelling. +static std::vector patchDiags(llvm::ArrayRef BaselineDiags, +const ScannedPreamble , I think this function is too long

[PATCH] D142890: [clangd] Add config option for fast diagnostics mode

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:235 MockFS FS; - auto TU = TestTU::withCode(Modified); + auto = PreambleTU; + TU.Code =

[PATCH] D144170: [clang-format] Add simple macro replacements in formatting.

sammccall added a comment. It's happening! Comment at: clang/lib/Format/ContinuationIndenter.cpp:743 + // Align following lines within parenthesis / brackets if configured. + // For a line of macro parents, the commas that follow the opening parenthesis

[PATCH] D143070: [clang-format] Enable FormatTokenSource to insert tokens.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Format/FormatTokenSource.h:74 public: - IndexedTokenSource(ArrayRef Tokens) + IndexedTokenSource(SmallVectorImpl ) :

[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/Preamble.cpp:691 +// it's coming from baseline preamble. +if (It->second) + PatchedInc = *It->second;

[PATCH] D140745: Generate Config {Fragment structure, json schema, docs, YAML parser} from schema spec

sammccall added a comment. Based on offline discussion, doubling down on checking in generated files to make these easy/possible to consume where they're needed. rebased and addressed high level comments (I think!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143070: [clang-format] Enable FormatTokenSource to insert tokens.

sammccall added a comment. Only serious concern is `getPreviousToken()`. Comment at: clang/lib/Format/FormatTokenSource.h:74 public: - IndexedTokenSource(ArrayRef Tokens) + IndexedTokenSource(SmallVectorImpl ) : Tokens(Tokens), Position(-1) {} As I

[PATCH] D143095: [clangd] Respect preamble-patch when handling diags

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/Diagnostics.cpp:102 + auto PatchedRange = [](CharSourceRange ) { +// FIXME: Should we handle all presumed locations? +

[PATCH] D143093: [clangd] #undef macros inside preamble patch

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/Preamble.cpp:216 unsigned Offset; + tok::PPKeywordKind Directive; + // Name of the macro being defined in the case of a

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

sammccall added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:648 + }); + if (NotePointsToOutside) +return true; Oh yeah, one more thing :-) Currently if the note/fix aren't at this line we're bailing out of the diagnostic entirely.

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ParsedAST.cpp:678 +if (Preamble) { + auto PDiags = Patch->patchedDiags(); + Diags->insert(Diags->end(), PDiags.begin(), PDiags.end()); llvm::append_range(Diags,

[PATCH] D143197: [clangd] Patch includes even without any changes

sammccall added a comment. Code looks reasonable, but I don't understand why the changes are being made - can you explain/link to a bug in the commit message? Comment at: clang-tools-extra/clangd/Preamble.cpp:688 if (It != ExistingIncludes.end()) { -

[PATCH] D142890: [clangd] Add config option for fast diagnostics mode

sammccall added a comment. The code looks good, but I have a very hard time following the tests. Comment at: clang-tools-extra/clangd/unittests/PreambleTests.cpp:225 auto TU = TestTU::withCode(Modified); + TU.AdditionalFiles = std::move(AdditionalFiles); auto CI =

[PATCH] D142467: [Tooling] Add stdlib::Symbol::all() and stdlib::Symbol::qualified_name()

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked 2 inline comments as done. Closed by commit rGe1aaa314a46c: [Tooling] Add stdlib::Symbol::all() and stdlib::Symbol::qualified_name() (authored by

[PATCH] D142467: [Tooling] Add stdlib::Symbol::all() and stdlib::Symbol::qualified_name()

sammccall marked 2 inline comments as done. sammccall added inline comments. Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:18 static llvm::StringRef *HeaderNames; +static struct SymbolName { hokein wrote: > nit: we group five symbols

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

sammccall added a comment. It looks like this fixes up the location only of diagnostics attached to particular directives (`#include`) based on some "deep" idea about the content of the directive (the spelled header name). Some shortcomings: - This misses diagnostics attached to other

[PATCH] D143093: [clangd] #undef macros inside preamble patch

sammccall added a comment. I can't understand from the description, code, or testcases what problem this is fixing. Can you clarify, ideally by improving the testcases? It seems to introduce a false negative in the case that the preamble *does* contain a definition of an already-defined macro,

[PATCH] D142890: [clangd] Add config option for fast diagnostics mode

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ConfigFragment.h:243 +/// - Strict +std::optional> Mode; + kadircet wrote: > sammccall wrote: > > I think "Diagnostics.Mode" is too vague a name. > > > > I expect this to be a rollout

[PATCH] D142890: [clangd] Add config option for fast diagnostics mode

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ConfigFragment.h:243 +/// - Strict +std::optional> Mode; + I think "Diagnostics.Mode" is too vague a name. I expect this to be a rollout flag that we remove in the medium term (either

[PATCH] D142871: [clangd] Semantic highlighting for constrained-parameter

sammccall added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:720 + // Pick up the name via VisitNamedDecl + Base::VisitTemplateTypeParmDecl(D); +} kadircet wrote: > nridge wrote: > > Am I using the API correctly here? It's a

[PATCH] D142637: A slightly more concise AST dump :)

sammccall added inline comments. Comment at: clang/test/AST/ast-dump-attr-type.cpp:12 +// CHECK-NEXT:`-BuiltinType 0x{{[^ ]*}} 'int' +// CHECK-NOT: `-PointerType 0x564c31a07520 'int *' gribozavr2 wrote: > sammccall wrote: > > Can you add a second

[PATCH] D142637: A slightly more concise AST dump :)

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/AST/ast-dump-attr-type.cpp:12 +// CHECK-NEXT:`-BuiltinType 0x{{[^ ]*}} 'int' +// CHECK-NOT: `-PointerType 0x564c31a07520 'int

[PATCH] D142467: [Tooling] Add stdlib::Symbol::all() and stdlib::Symbol::qualified_name()

sammccall updated this revision to Diff 491766. sammccall added a comment. oops, helps if I run tests on the right branch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142467/new/ https://reviews.llvm.org/D142467 Files:

[PATCH] D142467: [Tooling] Add stdlib::Symbol::all() and stdlib::Symbol::qualified_name()

sammccall created this revision. sammccall added a reviewer: VitaNuo. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. These address some remaining reasons to #include StdSymbolMap.inc directly.

[PATCH] D139921: [include-cleaner] Ranking of providers based on hints

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Herald added a subscriber: ChuanqiXu. LG, just nits really! Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:133 + +

[PATCH] D141580: [clang] Don't consider a nullptr returned from ActOnTag() as a valid result in ParseClassSpecifier.

sammccall added a comment. To offer the opposing argument: if DeclResult is just a bad idea, then using it consistently/right might be worse than using it as little as possible. FWIW, I find every piece of code that produces/consumes the `*Result` to be extremely confusing (the semantics of

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1feb7af04688: [clangd] support expanding `decltype(expr)` (authored by v1nh1shungry, committed by sammccall). Changed prior to commit:

[PATCH] D141537: [clangd] Tag clang-tidy diagnostics: modernize-*=deprecated, misc-unused-*=unneccesary

This revision was automatically updated to reflect the committed changes. Closed by commit rGbb1b0e61cda6: [clangd] Tag clang-tidy diagnostics: modernize-*=deprecated, misc-unused… (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D141280: [clang] Build UsingType for elaborated type specifiers.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Sema/DeclSpec.h:510 + // Returns the underlying decl, if any. Decl *getRepAsDecl() const { +auto *D = getRepAsFoundDecl();

[PATCH] D141175: [test] Split out Annotations from `TestingSupport`

sammccall accepted this revision. sammccall added a comment. This looks good, i think it also doesn't belong in Testing/Support because it's not a peer to anything in Support/. I think you can use `PARTIAL_SOURCES_INTENDED` to partition directories, but it's unusual and a bit of a hassle.

[PATCH] D141537: [clangd] Tag clang-tidy diagnostics: modernize-*=deprecated, misc-unused-*=unneccesary

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: kadircet, arphaman. Herald added a reviewer: njames93. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald

[PATCH] D140275: [clangd] Tweak to add doxygen comment to the function declaration

sammccall added a comment. Sorry about the delay on this. This looks useful - I know a lot of people write comments in this style, whatever my own reservations :-) Comment at: clang-tools-extra/clangd/refactor/tweaks/AddDoxygenComment.cpp:30 +// Given: +// int func(int x,

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

sammccall added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:151 + + // We shouldn't replace types like function and array, the commonality between + // these cases is that they use C-style declarator syntax that may have chunks

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LGTM with a comment nit, thank you! Do you have commit access? Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:151 + + // We shouldn't

[PATCH] D141495: [clangd] Suppress clang-tidy warnings for code spelled in system macros

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ParsedAST.cpp:545 } + // Match behavior for clang-tidy --system-headers=0 (the default). + if (Info.hasSourceManager() && nridge wrote: > Why not check

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

sammccall added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139 - // if it's a lambda expression, return an error message - if (isa(*DeducedType) && - cast(*DeducedType)->getDecl()->isLambda()) { -return error("Could

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

sammccall added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139 - // if it's a lambda expression, return an error message - if (isa(*DeducedType) && - cast(*DeducedType)->getDecl()->isLambda()) { -return error("Could

[PATCH] D141495: [clangd] Suppress clang-tidy warnings for code spelled in system macros

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf0e60f99aeff: [clangd] Suppress clang-tidy warnings for code spelled in system macros (authored by sammccall). Repository: rG LLVM Github

[PATCH] D141226: [clangd] support expanding `decltype(expr)`

sammccall added a comment. Thanks for doing this! There are a couple of separate logical changes here - I don't think it's a problem per se, but it does mean it takes a bit longer to get the good + obvious stuff reviewed and landed. Comment at:

[PATCH] D141495: [clangd] Suppress clang-tidy warnings for code spelled in system macros

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added a subscriber: arphaman. Herald added a reviewer: njames93. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a

[PATCH] D141478: [include-cleaner] Improve header spelling in the presence of links

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd7bba07526a7: [include-cleaner] Improve header spelling in the presence of links (authored by sammccall). Repository: rG LLVM Github Monorepo

[PATCH] D141478: [include-cleaner] Improve header spelling in the presence of links

sammccall added a comment. In D141478#4043134 , @hokein wrote: > Thanks for tracking it down. The solution looks good to me. Since this is a > fragile and subtle issue, is it possible to have a unittest for it? If it is > not too hard, it would be nice

[PATCH] D141478: [include-cleaner] Improve header spelling in the presence of links

sammccall created this revision. sammccall added reviewers: kadircet, hokein. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. HeaderSearch uses FileEntry::getName() to determine the best

[PATCH] D141362: [AST] include decls owned by FriendDecl in -ast-dump

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG98ae3616cd15: [AST] include decls owned by FriendDecl in -ast-dump (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D141362: [AST] include decls owned by FriendDecl in -ast-dump

sammccall created this revision. sammccall added a reviewer: hokein. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D141362 Files:

[PATCH] D141280: [clang][wip] Build UsingType for elaborated type specifiers.

sammccall added a comment. I'll bite, why do you say it's hacky? Looks OK to me, by clang standards :-) Comment at: clang/include/clang/Sema/DeclSpec.h:516 + } + Decl *getRepAsOriginDecl() const { +assert(isDeclRep((TST)TypeSpecType) && "DeclSpec does not store a decl");

[PATCH] D140551: [include-cleaner] Don't count references to operators as uses

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:75 +// doesn't count as uses (generally the type should provide them). +// Ignore them by

[PATCH] D140960: [clangd] Disable backend-releated filelist compiler options.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang-tools-extra/clangd/Compiler.cpp:84 + + // These options only affect the codegen in the backend, and clang will die + // immediately when

[PATCH] D140617: [clangd] Fix a clangd crash when indexing the standard library.

sammccall added a comment. As I understand, we're crashing if the ignorelist isn't found, and this patch fixes the searching so that we find the file (assuming the compile command is correct). But we'll still crash if the compile command is wrong, which seems pretty bad. Fixing the way the

[PATCH] D140462: [clangd] Add schema for `.clangd` config

sammccall added inline comments. Comment at: clang-tools-extra/clangd/schema/config.json:20 + "properties": { +"If": { + "description": "Conditions in the If block restrict when a fragment applies.", nridge wrote: > sammccall wrote: > > disabling

[PATCH] D140462: [clangd] Add schema for `.clangd` config

sammccall added a comment. The difficulty in testing is that: - JSON-schema is totally unknown in llvm-project, so there are no tools but also no expectation that contributors understand it - llvm-project doesn't like dependencies, particularly those that aren't in C++ or python - having done

[PATCH] D140462: [clangd] Add schema for `.clangd` config

sammccall added a comment. I think there's a clear benefit, but my feeling is it's outweighed by the costs, which are fairly high given lack of any automation/tests. If clangd contributors should maintain this, how should contributors/reviewers know whether changes are correct? If they needn't

[PATCH] D140551: [include-cleaner] Don't count references to operators as uses

sammccall added a comment. Yeah, I think this makes sense. I think this sort of thing should ideally be captured in the include-cleaner design doc if there was one. (Doesn't make sense to create one just for this issue, but it seems like a doc that should exist). Comment at:

[PATCH] D140462: [clangd] Add schema for `.clangd` config

sammccall added a comment. In D140462#4012451 , @nridge wrote: > Thanks, this is pretty neat! > > cc @sammccall as a heads up, just to make sure you're cool with having this > in-tree This has been discussed before, unfortunately while I *thought* it

[PATCH] D140532: .clang-tidy: disable misc-use-anonymous-namespace check.

sammccall added a comment. Yeah, good call. Do you mind if I first use this as a trigger to start a thread about this style rule on discourse, though? I've been meaning to, it came up in a review recently. I think there's a fair chance of changing the policy from "require static" to "allow

[PATCH] D140486: [clangd] Fix crashing race in ClangdServer shutdown with stdlib indexing

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rGb494f67f6796: [clangd] Fix crashing race in ClangdServer shutdown with stdlib indexing (authored by

[PATCH] D115232: [clangd] Indexing of standard library

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:1008-1010 + // TUScheduler is the only thing that starts background indexing work. + if (IndexTasks && !IndexTasks->wait(timeoutSeconds(TimeoutSeconds))) +return false;

[PATCH] D140486: [clangd] Fix crashing race in ClangdServer shutdown with stdlib indexing

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added a subscriber: arphaman. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. In

[PATCH] D115232: [clangd] Indexing of standard library

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:1008-1010 + // TUScheduler is the only thing that starts background indexing work. + if (IndexTasks && !IndexTasks->wait(timeoutSeconds(TimeoutSeconds))) +return false;

[PATCH] D140379: [clangd] Avoid triggering linkage computation for decl with unstable linkage in SymbolRelevanceSignals::computeASTSignals()

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang-tools-extra/clangd/Quality.cpp:306 if (const NamedDecl *ND = SemaResult.getDeclaration()) { +if (hasUnstableLinkage(ND)) +

[PATCH] D140300: [lit] Fix a few issues in relative_lines.py

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. thank you! I guess I tested this only on files with no matches/bad utf-8 :-( Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140300/new/

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