[PATCH] D50697: [clang-format] fix PR38557 - comments between "default" and ':' causes the case label to be treated as an identifier

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Repository: rC Clang https://reviews.llvm.org/D50697 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D47095: [clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name

klimek accepted this revision. klimek added a comment. LG Repository: rC Clang https://reviews.llvm.org/D47095 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38583: [clangd] Added async API to run code completion.

klimek added inline comments. Comment at: clangd/ClangdServer.cpp:222 + + std::shared_ptr Preamble = + Resources->getPossiblyStalePreamble(); I think we use "const type" everywhere. Comment at: clangd/ClangdServer.cpp:225 + // A task tha

[PATCH] D38583: [clangd] Added async API to run code completion.

klimek added inline comments. Comment at: clangd/ClangdServer.cpp:225 + // A task that will be run asynchronously. + PackagedTask Task([=]() mutable { // 'mutable' to reassign Preamble variable. +if (!Preamble) { ilya-biryukov wrote: > klimek wrote: > > It

[PATCH] D38583: [clangd] Added async API to run code completion.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Comment at: clangd/ClangdServer.h:236-237 + /// + /// Request is processed asynchronously, you can use returned future to wait + /// for results of async request. + /

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

klimek added inline comments. Comment at: include/clang/Tooling/CommonOptionsParser.h:90 /// - /// This constructor exits program in case of error. + /// If \p ExitOnError is set (default), This constructor exits program in case + /// of error; otherwise, this sets the err

[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes

klimek added a comment. Whee, thanks for driving this, much appreciated :D Comment at: include/clang/FrontendTool/Utils.h:25 + +std::unique_ptr CreateFrontendAction(CompilerInstance &CI); Please add a comment now that this is exported. Comm

[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes

klimek added a comment. Ok, I think this is starting to look really good. Now we should try to grab Richard's attention to make sure it's fine to submit with the current set of FIXMEs. Comment at: lib/Frontend/FrontendActions.cpp:386 + // This part is normally done by ASTFro

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

klimek added inline comments. Comment at: include/clang/Tooling/Execution.h:76 + + void appendArgumentsAdjuster(ArgumentsAdjuster Adjuster); + I think the argument adjuster adjustment shouldn't be part of this interface, as the argument adjusters cannot be chan

[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

klimek added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1307 + (Style.BraceWrapping.BeforeLambdaBody && Current.Next != nullptr && +Current.Next->is(TT_LambdaLSquare))); State.Stack.back().IsInsideObjCArrayLiteral = klimek w

[PATCH] D51261: Add preload option to clang-query

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51261 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D51259: Allow binding to NamedValue resulting from let expression

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rC Clang https://reviews.llvm.org/D51259 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D51258: Extract parseBindID method

klimek added inline comments. Comment at: lib/ASTMatchers/Dynamic/Parser.cpp:362 +bool Parser::parseBindID(std::string &BindID, TokenInfo &CloseToken) { + // Parse .bind("foo") CloseToken seems to not be used afterwards either here or in the follow-up patch?

[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

klimek added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1307 + (Style.BraceWrapping.BeforeLambdaBody && Current.Next != nullptr && +Current.Next->is(TT_LambdaLSquare))); State.Stack.back().IsInsideObjCArrayLiteral = Wawha wr

[PATCH] D49724: [VFS] Cleanups to VFS interfaces.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Repository: rC Clang https://reviews.llvm.org/D49724 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D49840: [AST] Add MatchFinder::matchSubtree

klimek added a comment. Usually we use match(anyOf(node), hasDescendant(node)). Or did I misunderstand what you want? Repository: rC Clang https://reviews.llvm.org/D49840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D50078: clang-format: support aligned nested conditionals formatting

klimek added a comment. Yea, if we go down this route I'd go with this by default: some ? thing : else ? otherthing : unrelated ? but : finally; Theoretically we could even use: some ? thing : else; by default ;) Repository: rC Clang https://reviews.llvm.org/D50078

[PATCH] D37813: clang-format: better handle namespace macros

klimek added a comment. The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently. Given that you want full control over the formatting of t

[PATCH] D45719: [clang-Format] Fix indentation of member call after block

klimek added a comment. Sorry that I lost track of that, but feel free to ping once / week - unfortunately the patch doesn't apply cleanly, can you rebase? Repository: rC Clang https://reviews.llvm.org/D45719 ___ cfe-commits mailing list cfe-com

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

klimek added a comment. I'm less concerned about everything suddenly re-indenting when you change code - if you use any kind of namespace indentation, that's what will happen now and then (and is why many style guides do not indent in namespaces). For what it's worth, I'd 1. vote for only merg

[PATCH] D33714: clang-format: [JS] improve calculateBraceType heuristic

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. Apart from fixme LG. Comment at: lib/Format/UnwrappedLineParser.cpp:397 // blocks (for example while parsing lambdas). + // TODO(martinprobst): Some of th

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

klimek added a comment. In https://reviews.llvm.org/D32480#773807, @Typz wrote: > So how do I proceed? > > 1. Keep the CompactNamespace option, and make "compacted" namespaces always > add at most one level of indentation > 2. Or assume that this can only ever usefully work with the behavior of

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

klimek added a comment. Generally LG from my side. Comment at: unittests/Format/FormatTest.cpp:1363-1381 + EXPECT_EQ("namespace {\n" + "namespace {\n" +"}} // namespace aaa

[PATCH] D33644: Add default values for function parameter chunks

klimek added a comment. Can you give a bit more background what this is trying to do? https://reviews.llvm.org/D33644 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33644: Add default values for function parameter chunks

klimek added a comment. Ok, now I get it - can you please add tests? This is usually tested by adding a c-index-test based test. Comment at: lib/Sema/SemaCodeComplete.cpp:2411-2417 + const SourceLocation StartLoc = SrcRange.getBegin(); + const SourceLocation EndLoc = SrcRang

[PATCH] D33644: Add default values for function parameter chunks

klimek added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:2453 std::string PlaceholderStr = FormatFunctionParameter(Policy, Param); +if (Param->hasDefaultArg() && PlaceholderStr.find("=") == std::string::npos) { +std::string DefaultValue =

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that. https://reviews.llvm.org/D34304

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

klimek added a comment. Generally this patch lg from my side - how many patches for single file mode are coming down the road, though? I'm somewhat concerned about the overall complexity it'll add to clang. When I saw your first patch my reaction was "wow, this works with this little change - c

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

klimek added inline comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:42-43 +/// destructors. +/// An assertion will fire if two PCHTempFiles are created with the same name, +/// so it's not intended to be used outside preamble-handling. +class TempPCHFile {

[PATCH] D33823: [clang-format] Support sorting using declarations

klimek added inline comments. Comment at: lib/Format/UsingDeclarationsSorter.cpp:41-42 + +bool computeUsingDeclarationLabel(const FormatToken *UsingTok, + std::string *Label) { + assert(UsingTok && UsingTok->is(tok::kw_using) && "Expecting a usin

[PATCH] D33644: Add default values for function parameter chunks

klimek added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:2453 std::string PlaceholderStr = FormatFunctionParameter(Policy, Param); +if (Param->hasDefaultArg() && PlaceholderStr.find("=") == std::string::npos) { +std::string DefaultValue =

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:123 + +void runDiff(ASTContext &AST1, ASTContext &AST2); + This is the main exposed interface? Generally, if all we want to do is printing, I wouldn't put that into a library in T

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. In https://reviews.llvm.org/D34304#784496, @saugustine wrote: > In https://reviews.llvm.org/D34304#783675, @klimek wrote: > > > I think a better way might be to generally leave dependency options alone, > > add a default argument adapter to filter out all deps related fla

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. oh, and lg from my side https://reviews.llvm.org/D34263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mai

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

klimek added a comment. In https://reviews.llvm.org/D34263#784168, @akyrtzi wrote: > In https://reviews.llvm.org/D34263#783694, @klimek wrote: > > > how many patches for single file mode are coming down the road, though? I'm > > somewhat concerned about the overall complexity it'll add to clang.

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

klimek added inline comments. Comment at: lib/Frontend/ASTUnit.cpp:131-136 +/// \brief Get a source buffer for \p MainFilePath, handling all file-to-file +/// and file-to-buffer remappings inside \p Invocation. +static PossiblyOwnedBuffer +getBufferForFileHandlingRemapping(const

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

klimek added inline comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:124 +/// CanReusePreamble + AddImplicitPreamble to make use of it. +class PrecompiledPreamble { +public: ilya-biryukov wrote: > klimek wrote: > > If a user doesn't care about t

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added a comment. Reviewing this mainly from the API view, and leaving the technical details to others :) Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:124 + +class ASTDiff { + TreeRoot &T1, &T2; Generally, can we put the public interface first in

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D34287 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D33823: [clang-format] Support sorting using declarations

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D33823 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate? https://reviews.llvm.org/D34304 ___ cfe-commits mailing lis

[PATCH] D34470: [clangd] Allow to override resource dir in ClangdServer.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D34470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D34469: Use vfs::FileSystem in ASTUnit when creating CompilerInvocation.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D34469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57 +/// Within a tree, this identifies a node by its preorder offset. +using NodeId = int; + johannes wrote: > arphaman wrote: > > I think that it's better to make make `NodeId` a s

[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling

klimek added a comment. General direction looks good. I think in addition to integration tests through the static analyzer, we should still have unit tests. Comment at: include/clang/Tooling/CrossTranslationUnit.h:47 + + const FunctionDecl *getCTUDefinition(const FunctionDec

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. In https://reviews.llvm.org/D34304#788080, @saugustine wrote: > In https://reviews.llvm.org/D34304#787699, @klimek wrote: > > > I mean, arguments need to be adjusted before converting to ArgStringList > > and calling newInvocation? I'm not sure I fully understand the prob

[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling

klimek added reviewers: dexonsmith, akyrtzi, benlangmuir, arphaman. klimek added a comment. Adding folks interested in the indexing discussion. Comment at: include/clang/Tooling/CrossTranslationUnit.h:53-58 + /// \p CrossTUDir directory, called \p IndexName. In case the declar

[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling

klimek added inline comments. Comment at: include/clang/Tooling/CrossTranslationUnit.h:53-58 + /// \p CrossTUDir directory, called \p IndexName. In case the declaration is + /// found in the index the corresponding AST file will be loaded and the + /// definition of the functi

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

klimek added a reviewer: rsmith. klimek added a comment. Richard (added as reviewer) usually owns decisions around clang itself. Writing an email to cfe-dev with the numbers and wait for whether others have concerns would probably also be good. https://reviews.llvm.org/D30691 __

[PATCH] D37813: clang-format: better handle namespace macros

klimek added a comment. I've talked with Daniel and we both believe this patch is not the right way to go forward for clang-format. It is adding configuration mechanisms that we need to maintain going forward, without addressing a general problem; specifically for how to handle macros, I believ

[PATCH] D37813: clang-format: better handle namespace macros

klimek added a comment. In https://reviews.llvm.org/D37813#958116, @Typz wrote: > OK. > > So you mean a solution like the one discussed earlier would be the way to go? > > > I mean that we can configure macros in the format style, like "define A(X) > > class X {". I'm not 100% sure whether we wo

[PATCH] D41487: [clang-format] Adds a FormatStyleSet

klimek added inline comments. Comment at: lib/Format/Format.cpp:893 for (int i = Styles.size() - 1; i >= 0; --i) { -if (Styles[i].Language == Language || -Styles[i].Language == FormatStyle::LK_None) { +if (!LanguageFound && (Styles[i].Language == Language || +

[PATCH] D41487: [clang-format] Adds a FormatStyleSet

klimek added inline comments. Comment at: lib/Format/Format.cpp:906-907 + } + if (!LanguageFound) +return make_error_code(ParseError::Unsuitable); + *Style = *StyleSet.Get(Language); Optional: I'd probably slightly re-structure the above to: if (!Languag

[PATCH] D46024: [clang-format] Add SpaceBeforeCpp11BracedList option.

klimek accepted this revision. klimek added a comment. In https://reviews.llvm.org/D46024#1129350, @hans wrote: > In https://reviews.llvm.org/D46024#1121242, @rkirsling wrote: > > > FWIW, please note that this space-before-brace style is not specific to > > WebKit; CppCoreGuidelines exhibits it

[PATCH] D47759: [Format] Do not use a global static value for EOF within ScopedMacroState.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG. Repository: rC Clang https://reviews.llvm.org/D47759 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D48242: [ASTMatchers] Add support for matching the type of a friend decl.

klimek added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:2904 +internal::Matcher, InnerMatcher, 1) { + QualType QT = internal::getUnderlyingType(Node); + if (!QT.isNull()) In which cases can getUnderlyingType return a null type? R

[PATCH] D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab

klimek added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:678 // Indent with tabs only when there's at least one full tab. -if (FirstTabWidth + Style.TabWidth <= Spaces) { +if (Style.TabWidth <= Spaces) { Spaces -= FirstTabWidth; -

[PATCH] D48269: [ASTMatchers] Don't assert-fail in specifiesTypeLoc().

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg, thanks! Repository: rC Clang https://reviews.llvm.org/D48269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D48242: [ASTMatchers] Add support for matching the type of a friend decl.

klimek accepted this revision. klimek added a comment. lg, thx Repository: rC Clang https://reviews.llvm.org/D48242 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab

klimek added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:678 // Indent with tabs only when there's at least one full tab. -if (FirstTabWidth + Style.TabWidth <= Spaces) { +if (Style.TabWidth <= Spaces) { Spaces -= FirstTabWidth; -

[PATCH] D45719: [clang-Format] Fix indentation of member call after block

klimek added inline comments. Comment at: unittests/Format/FormatTest.cpp:4359 + "return 3;\n" + " }).as("");\n" + "}"); ank wrote: > klimek wrote: > > What would be interesting is tests that: > > a) have another val

[PATCH] D56841: [clangd] Filter out plugin related flags.

klimek added a comment. In D56841#1361395 , @kadircet wrote: > It seems like the most relevant place in tooling is ArgumentsAdjuster as > @ioeric pointed out. Happy to move it to there if @sammccall and @klimek > agrees as well. > > As a side note `Argum

[PATCH] D54077: [clangd] Implemented DraftFileSystem

klimek added a comment. In https://reviews.llvm.org/D54077#1287040, @ilya-biryukov wrote: > Thanks for the patch! I believe many people I talked to want this behavior > (myself included). > Some people like what we do now more. It feels like it depends on the > workflow: for people who auto-sa

[PATCH] D54077: [clangd] Implemented DraftFileSystem

klimek added a comment. In https://reviews.llvm.org/D54077#1287282, @LutsenkoDanil wrote: > @klimek If behavior will be configurable, is it ok for you? I have the same concerns as Sam for making this an option. > @sammccall Current behavior may confuse new users, since, other IDEs mostly > (a

[PATCH] D54077: [clangd] Implemented DraftFileSystem

klimek added a comment. In https://reviews.llvm.org/D54077#1288404, @ioeric wrote: > In https://reviews.llvm.org/D54077#1288387, @sammccall wrote: > > > Someone mentioned to me that the interaction-between-features argument > > wasn't clear here: > > > > - we **don't** currently update diagnosti

[PATCH] D54453: Remove myself as owner of clang-query.

klimek added a comment. Thanks Aaron for volunteering, I'm very thankful for all your work on the reviews, it's much appreciated :D Repository: rL LLVM https://reviews.llvm.org/D54453 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D54952: [clangd] DO NOT SUBMIT. Draft interfaces for build system integration.

klimek added inline comments. Comment at: clangd/BuildSystem.h:29 +/// Default compilation database used by clangd, based on the build system. +class Integration : public GlobalCompilationDatabase { +public: 'Integration' is a bit of a weird name, as it doesn't t

[PATCH] D54952: [clangd] DO NOT SUBMIT. Draft interfaces for build system integration.

klimek added inline comments. Comment at: clangd/BuildSystem.h:29 +/// Default compilation database used by clangd, based on the build system. +class Integration : public GlobalCompilationDatabase { +public: ilya-biryukov wrote: > klimek wrote: > > 'Integration'

[PATCH] D54672: clang-include-fixer.el: support remote files

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54672/new/ https://reviews.llvm.org/D54672 ___ cf

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

klimek added a comment. In D54309#1326852 , @JonasToth wrote: > See PR39949 as well, as it seems to trigger the same or a similar problem. > @ioeric and @klimek maybe could give an opinion, too? Sounds like we might not correctly set the parent map for

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

klimek added a comment. Why did the numeric constant break this? Which path would the function run through? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59309/new/ https://reviews.llvm.org/D59309 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D33029: [clang-format] add option for dangling parenthesis

klimek added a comment. In D33029#1423949 , @MyDeveloperDay wrote: > In D33029#1423947 , @lebedev.ri > wrote: > > > In D33029#1423944 , > > @MyDeveloperDay wrote: > > > >

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

klimek added a comment. In D59309#1428807 , @MyDeveloperDay wrote: > In D59309#1428799 , @klimek wrote: > > > Why did the numeric constant break this? Which path would the function run > > through? > > > as its lo

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1423941 , @MyDeveloperDay wrote: > In D55170#1423798 , @reuk wrote: > > > Thanks for the review, and for approving this PR. It's very much > > appreciated! > > > > I don't have me

[PATCH] D52150: [clang-format] BeforeHash added to IndentPPDirectives

klimek accepted this revision. klimek added a comment. Given this doesn't add an extra option (but only an extra enum) and is fairly straight forward, LG. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52150/new/ https://reviews.llvm.org/D52150 ___

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added a comment. In D28462#1406716 , @jacob-keller wrote: > In regards to whether clang-format should support this feature, I think it's > pretty clear that a large number of users want it to. The tool already > supports formatting various other

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

klimek added a comment. In D59309#1428969 , @MyDeveloperDay wrote: > That works because the argument list is just tok::identifier and > TT_StartOfName Should we fix isStartOfName then? In A or A<8> A should be TT_StartOfName, too, right? CHANGES SIN

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

klimek added a comment. Yea, the a b(c) problem is the olden C++ problem clang-format will never be able to solve, thus all these crazy heuristics. So, inching closer to understanding this - I totally missed the isLiteral() that short-circuits out before. I'm more happy with your current approa

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

klimek added a comment. In D40988#1430502 , @MyDeveloperDay wrote: > So @russellmcc you've been bumping along this road nicely for 6 months, > doing what people say... pinging every week or so in order to get your code > reviewed, and you are getting n

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1431854 , @MyDeveloperDay wrote: > > I'm sorry for not having a positive answer here, but I'm not in favor of > > this change. The style rule looks like it introduces arbitrary complexity > > at a point where I don't un

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1432832 , @reuk wrote: > @klimek I agree that the rule is somewhat arbitrary. However, it's the style > rule of an established codebase with many users (I don't have a concrete > number, but the project has 1400 stars on

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1432929 , @MyDeveloperDay wrote: > > I don't think risk is what matters - in the end it's about cost, and cost > > is a very technical reason > > I'm really sorry I'm not trying to be difficult its just over the years I

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2546-2560 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) || (Style.SpaceBeforeParens != FormatStyle::SBPO_Never && -(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok

[PATCH] D58404: [clang-format] Add basic support for formatting C# files

klimek added inline comments. Comment at: include/clang/Basic/TokenKinds.h:80 K == tok::utf8_string_literal || K == tok::utf16_string_literal || - K == tok::utf32_string_literal; } This change looks pretty good, minus the changes outside Format

[PATCH] D59408: [clang-format] [PR25010] Extend AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

klimek added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:401 + * ``SIS_AlwaysNoElse`` (in configuration: ``AlwaysNoElse``) +Allow short if/else if statements even if the else is a compound statement. + I'd try to make this either unco

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:436 +static void +AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column, + F &&Matches, VelocityRa wrote: > klimek wrote: > > I don't think the 'All' pos

[PATCH] D57687: [clang-format] Add style option AllowShortLambdasOnASingleLine

klimek added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:2976 + +return Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_None || + Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_Inline || If SLS_All is supposed

[PATCH] D59546: [clang-format] structured binding in range for detected as Objective C

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59546/new/ https://reviews.llvm.org/D59546 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

klimek added a comment. I don't understand yet why running clang-format locally would not do the same change? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54881/new/ https://reviews.llvm.org/D54881 ___ cfe-commits mailing list cfe-commits

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2546-2560 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) || (Style.SpaceBeforeParens != FormatStyle::SBPO_Never && -(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok

[PATCH] D42034: [clang-format] In tests, expected code should be format-stable

klimek added a comment. Great idea! LG from my side after addressing MyDeveloperDay's comments. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D42034/new/ https://reviews.llvm.org/D42034 ___ cfe-commits mailing list cfe

[PATCH] D58404: [clang-format] Add basic support for formatting C# files

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG. This looks pretty sharp (scnr). Comment at: clang/lib/Format/FormatTokenLexer.cpp:177 +if (Dollar->TokenText == "$") { + // this looks like $@"a" so we need

[PATCH] D57687: [clang-format] Add style option AllowShortLambdasOnASingleLine

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57687/new/ https://reviews.llvm.org/D57687 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:482 + // containing any matching token to be aligned and located after such token. + auto AlignCurrentSequence = [&] { +if (StartOfSequence > 0 && StartOfSequence < EndOfSequence) {

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1439864 , @MyDeveloperDay wrote: > @lebedev.ri Are we talking about a general ideology of the long term cost to > allow any new things in? or to not allow things in this specific case? > > because in this specific case a

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek accepted this revision. klimek added a comment. Oh, and LG :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org/D55170 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

klimek added a comment. Minus me understanding the TT_TemplateString change, the rest looks nice now, thanks! Comment at: clang/lib/Format/TokenAnnotator.cpp:3177-3178 return false; // Don't split tagged template literal so there is a break between the tag //

[PATCH] D59684: [clang-format] [PR41187] moves Java import statements to the wrong location if code contains statements that start with the word import

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59684/new/ https://reviews.llvm.org/D59684 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D59774: [clang-format] Refine structured binding detection

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59774/new/ https://reviews.llvm.org/D59774 ___ cfe-commits mail

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

klimek added a comment. I just verified why this doesn't break for our CI: The trick is that if the indent is actually off (as opposed to being correct, but tab vs spaces), we do re-indent until we find an indent that's correct. The fix that would make me happier, I think, is to do the same thing

  1   2   3   4   5   6   >