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

[Manuel Klimek via Phabricator via cfe-commits]

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/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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.

[Manuel Klimek via Phabricator via cfe-commits]

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 that will be run asynchronously.
+  PackagedTask Task([=]() mutable { // 'mutable' to reassign Preamble variable.
+if (!Preamble) {

It doesn't seem like we use Preamble anywhere else but in the lambda - so why 
not get it in the lambda?


https://reviews.llvm.org/D38583



___
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.

[Manuel Klimek via Phabricator via cfe-commits]

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 doesn't seem like we use Preamble anywhere else but in the lambda - so 
> > why not get it in the lambda?
> TLDR; To use in async requests exactly the same preamble that was previously 
> used for sync requests.
> 
> Those are two different time points. Our callers might change the file when 
> we'll be executing this lambda. I assume that most of time we'll want the 
> preamble that was built at the point where `codeComplete` is called, not 
> after that.
> 
> On the other hand, after file was changed, code completion results will 
> probably be irrelevant and will be discarded by clients anyway, so that might 
> not matter. I've opted for not changing the behavior and using the same 
> preamble that was previously used by the synchronous version. (Unless 
> `Preamble` was null in the sync case, where we would only improve 
> performance).
That makes sense, but needs a comment in the code :)



Comment at: clangd/ClangdServer.h:236-237
+  ///
+  /// Request is processed  asynchronously, you can use returned future to wait
+  /// for results of async request.
+  ///

Extra space before asynchronously.
Replace first ',' with ';'
... wait for the results of the async request...


https://reviews.llvm.org/D38583



___
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.

[Manuel Klimek via Phabricator via cfe-commits]

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.
+  ///

ilya-biryukov wrote:
> klimek wrote:
> > Extra space before asynchronously.
> > Replace first ',' with ';'
> > ... wait for the results of the async request...
> Split into two sentences on `,` instead of replacing it with `;`. Feels even 
> better.
..."the" returned future...



https://reviews.llvm.org/D38583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 error flag and stores error messages.

hokein wrote:
> `... is set to true` for clarify.
If we want error handling, why not make it a static factory that returns an 
ErrorOr instead?



Comment at: include/clang/Tooling/Execution.h:26-27
+//
+//  This is still experimental but expected to replace the existing `ClangTool`
+//  interface.
+//

I'd remove this line. Those tend to only get stale without anybody changing 
them :)



Comment at: include/clang/Tooling/Execution.h:49
+public:
+  virtual ~ToolResults() {}
+  virtual void addResult(StringRef Key, StringRef Value) = 0;

I think = default is the new cool way.



Comment at: include/clang/Tooling/Execution.h:51-53
+  virtual std::vector> AllKVResults() = 0;
+  virtual void forEachResult(
+  llvm::function_ref Callback) = 0;

Why do we need to get the results via the interface? For example, in a 
map/reduce style setup getting the results is infeasible.



Comment at: include/clang/Tooling/Execution.h:76
+
+  virtual ToolResults *getToolResults() const { return Results; }
+

I think it's weird that this is virtual, but we also  have a default 
implementation that returns something that we require in the constructor. 
Either make that virtual, and don't put in a default implementation, or make it 
non-virtual.



Comment at: include/clang/Tooling/Execution.h:90
+private:
+  // A reference to the results container. Not owned!
+  ToolResults *Results;

I don't think we need to annotate every unowned pointer. Pointers are unowned 
by default, otherwise we'd use unique_ptr.



Comment at: include/clang/Tooling/Execution.h:164-166
+createExecutorFromCommandLineArgs(int &argc, const char **argv,
+  llvm::cl::OptionCategory &Category,
+  const char *Overview = nullptr);

I think for clang-refactor we'll need one level of abstraction in the middle:
We'll need to be able to say "run on all translation units referencing symbol 
X" - how will this fit into the design?
Also, how will code changes generally fit with this design?


https://reviews.llvm.org/D34272



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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.



Comment at: include/clang/Sema/TemplateInstCallbacks.h:32
+  /// \brief Called before doing AST-parsing.
+  void initialize(const Sema &TheSema) {
+this->initializeImpl(TheSema);

Note sure how others think about this, but I'd have expected the design to have:
a) a pure interface
b) an implementation TemplateInstantiationCallbacksCollection or somesuch that 
has a vector of TICs and forwards calls on them


https://reviews.llvm.org/D5767



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 ASTFrontEndAction, but needs to happen
+  //  before Templight observers can be created --->>
+  // FIXME: Move the truncation aspect of this into Sema, we delayed this till

Usually we don't use ascii art. Perhaps instead of marking a scope with arrows, 
pull out a small function that explains what we want to do instead?


https://reviews.llvm.org/D5767



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 changed in the phase in which we want the 
ExecutionContext to be used.

I'd just make the argument adjusters a parameter on the constructor here (or 
alternatively, do not couple them in here, and just hand them around as a 
separate entity).



Comment at: include/clang/Tooling/Execution.h:130-131
+  /// context.
+  llvm::Error execute(ArgumentsAdjuster Adjuster,
+  std::unique_ptr Action);
+

I'd put the ArgumentAdjust second (as the action is the primary thing being 
acted on).



https://reviews.llvm.org/D34272



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 wrote:
> I think I misunderstood some of this the first time around (and thanks for 
> bearing with me :) - iiuc we want to break for Allman even when there's only 
> one nested block. I think I'll need to take another look when I'm back from 
> vacation, unless Daniel or somebody else finds time before then (will be back 
> in 1.5 weeks)
So, HasMultipleNestedBlocks should only be true if there are multiple nested 
blocks.
What tests break if you remove this change to HasMultipleNestedBlocks?


Repository:
  rC Clang

https://reviews.llvm.org/D44609



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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/mailman/listinfo/cfe-commits

[PATCH] D51258: Extract parseBindID method

[Manuel Klimek via Phabricator via cfe-commits]

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?


Repository:
  rC Clang

https://reviews.llvm.org/D51258



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 wrote:
> klimek wrote:
> > klimek wrote:
> > > I think I misunderstood some of this the first time around (and thanks 
> > > for bearing with me :) - iiuc we want to break for Allman even when 
> > > there's only one nested block. I think I'll need to take another look 
> > > when I'm back from vacation, unless Daniel or somebody else finds time 
> > > before then (will be back in 1.5 weeks)
> > So, HasMultipleNestedBlocks should only be true if there are multiple 
> > nested blocks.
> > What tests break if you remove this change to HasMultipleNestedBlocks?
> All cases with only one lambda in parameter are break. The Lambda body with 
> be put on the same line as the function and aligned with [] instead of 
> putting the body [] on another line with just one simple indentation.
> 
> So if restore the line to :
> 
> ```
> State.Stack.back().HasMultipleNestedBlocks = Current.BlockParameterCount > 1;
> ```
> 
> Here some cases.
> FormatTest.cpp, ligne 11412:
> 
> ```
> // With my modification
> FunctionWithOneParam(
> []()
> {
>   // A cool function...
>   return 43;
> });
> 
> // Without my modification
> FunctionWithOneParam([]()
>  {
>// A cool function...
>return 43;
>  });
> ```
> The "{" block of the lambda will be aligned on the "[" depending of the 
> function name length.
> 
> 
> You have the same case with multiple lambdas (FormatTest.cpp, ligne 11433):
> 
> ```
> // With my modification
> TwoNestedLambdas(
> []()
> {
>   return Call(
>   []()
>   {
> return 17;
>   });
> });
> 
> // Without my modification
> TwoNestedLambdas([]()
>  {
>return Call([]()
>{
>  return 17;
>});
>  });
> ```
Do we want to always break before lambdas in Allman style?


Repository:
  rC Clang

https://reviews.llvm.org/D44609



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 those macros, and them 
not actually be formatted exactly like namespaces or classes, I think we need a 
more generic mechanism for you to express that.


Repository:
  rC Clang

https://reviews.llvm.org/D37813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 merging in cases where both start and end can be merged
2. vote for merge namespaces counting as a single namespace decl


https://reviews.llvm.org/D32480



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 these do not apply to JS, e.g. "} {"
+  // can never be a braced list in JS.

Here we call them FIXME and put them without a name:
// FIXME: ...


https://reviews.llvm.org/D33714



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 
> NI_None and add an additional enum value NI_Compact.


I'd vote for (1).

> And should we limit 'compacting' to namespace which start and end at 
> consecutive lines [e.g. exactly the same conditions as C++17 nested 
> namespaces], or is the current implementation enough [merge all adjacent 
> beginning and all adjacent endings] ?

I'd vote for the former.


https://reviews.llvm.org/D32480



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Generally LG from my side.




Comment at: unittests/Format/FormatTest.cpp:1363-1381
+  EXPECT_EQ("namespace  {\n"
+   "namespace  {\n"
+"}} // namespace 
::",
+format("namespace  {\n"
+   "namespace  {\n"
+   "} // namespace \n"
+   "} // namespace ",

These tests become more readable if you set up a style with a smaller column 
limit.


https://reviews.llvm.org/D32480



___
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

[Manuel Klimek via Phabricator via cfe-commits]

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

[Manuel Klimek via Phabricator via cfe-commits]

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 = SrcRange.getEnd();
+  if (StartLoc != SM.getExpansionLoc(StartLoc) || EndLoc != 
SM.getExpansionLoc(EndLoc))
+  return "";
+  const size_t PtrDiff = EndLoc.getRawEncoding() - StartLoc.getRawEncoding()
+  + Lexer::MeasureTokenLength(EndLoc, SM, LangOpts);
+  return std::string{SM.getCharacterData(StartLoc), PtrDiff};

Can you use Lexer::getSourceText instead?



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

Why the check for = in the PlaceholderStr?


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

[Manuel Klimek via Phabricator via cfe-commits]

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 =

yvvan wrote:
> klimek wrote:
> > Why the check for = in the PlaceholderStr?
> Not to add default value twice. If there's already "=" in placeholder string 
> that means we've already added it in FormatFunctionParameter call
In which cases would the default value not be added in FormatFunctionParameter 
if there is one, and need to be added here?


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] D34304: Allow CompilerInvocations to generate .d files.

[Manuel Klimek via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 - cool".
With this patch it's "well, that doesn't seem to add too much complexity, so 
looks good".
I don't want to realize 5 patches down the line that we're sprinkling all of 
clang with single file conditional code without some discussion about the 
strategy with Richard.




Comment at: include/clang/Lex/Preprocessor.h:1838-1841
+  struct DirectiveEvalResult {
+bool Conditional;
+bool IncludedUndefinedIds;
+  };

A short comment on the struct what the semantics are would be helpful.


https://reviews.llvm.org/D34263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 {

a) s/PCHTempFile/TempPCHFile/
b) is the unique-ness the only thing that's special? Perhaps UniqueTempFile or 
somesuch? Seems nothing PCH-specific is here to see
c) why don't we want to support VFS's here?



Comment at: include/clang/Frontend/PrecompiledPreamble.h:97
+  /// All files have size set.
+  off_t Size;
+

I'd initialize these.



Comment at: include/clang/Frontend/PrecompiledPreamble.h:124
+/// CanReusePreamble + AddImplicitPreamble to make use of it.
+class PrecompiledPreamble {
+public:

If a user doesn't care about the things above this class, can we move those 
into an extra header?



Comment at: include/clang/Frontend/PrecompiledPreamble.h:142
+private:
+  /// Manages a lifetime of temporary file that stores PCH.
+  TempPCHFile PCHFile;

the lifetime ... stores a PCH



Comment at: include/clang/Frontend/PrecompiledPreamble.h:157-160
+  /// We don't expose PCHFile to avoid changing interface when we'll add an
+  /// in-memory PCH, so we declare this function as friend so that it has 
access
+  /// to PCHFile field.
+  friend void AddImplicitPreamble(CompilerInvocation &CI,

Why not make it a member instead?



Comment at: lib/Frontend/ASTUnit.cpp:98
 
-static llvm::sys::SmartMutex &getOnDiskMutex() {
-  static llvm::sys::SmartMutex M(/* recursive = */ true);
-  return M;
-}
+  /// A helper class, storing a either a raw pointer, or unique_ptr to an 
llvm::MemoryBuffer.
+  struct PossiblyOwnedBuffer {

I'd instead just have both a unique_ptr for memory management and a pointer 
(for unowned access). Then, if owned, make the raw pointer point to the 
unique_ptr, if unowned, the unique_ptr stays nullptr.



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 CompilerInvocation &Invocation,
+  vfs::FileSystem *VFS,
+  StringRef FilePath) {

If this indeed needs to return a possibly owned buffer, explain when exactly it 
is owned and when it is unowned.


https://reviews.llvm.org/D34287



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 using token");

Use either:
- an empty label to indicate that there is no more label, and thus we couldn't 
compute one
- use an llvm::ErrorOr as return value



Comment at: lib/Format/UsingDeclarationsSorter.cpp:59
+Tok = Tok->Next;
+if (Tok && Tok->is(tok::coloncolon)) {
+  Label->append("::");

I'd invert this if - the happy case is in the if here. That way, you also don't 
need a continue.


https://reviews.llvm.org/D33823



___
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

[Manuel Klimek via Phabricator via cfe-commits]

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 =

yvvan wrote:
> klimek wrote:
> > yvvan wrote:
> > > klimek wrote:
> > > > Why the check for = in the PlaceholderStr?
> > > Not to add default value twice. If there's already "=" in placeholder 
> > > string that means we've already added it in FormatFunctionParameter call
> > In which cases would the default value not be added in 
> > FormatFunctionParameter if there is one, and need to be added here?
> If Param->evaluateValue() can evaluate it it will set the default value in 
> FormatFunctionParameter (that means it's a primitive type like "void func(int 
> i = 0)").
> In case it's some kind of non-primitive type like "void func(Foo foo = Foo(0, 
> 0))" it will not be evaluated and we can use here the source manager to get 
> the default value string. In this example it will be Foo(0, 0).
Why don't we always add it in the unevaluated form? I'd expect constants from 
macros are useful to see unevaluated?


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] D34329: [GSoC] Clang AST diffing

[Manuel Klimek via Phabricator via cfe-commits]

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 Tooling, but just implement a tools/ASTDiffer or somesuch.

If you want to make this a library, it should return the diff in some form 
that's nice to use (or print).



https://reviews.llvm.org/D34329



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 flags, and 
> > allow users to add their own argument adapters that don't do that.
>
>
> This argument adapter would have to be passed down in a similar way, no?
>
> buildASTFromCodeWithArgs, toolIinvocation::Run, and newInvocation are all 
> entry-points that would need this behavior, and all are called by themselves 
> in one place or another.


I think it's cleaner, because it uses a concept we already have (argument 
adapters).

> I'm happy to do that, as it is quite a bit more flexible, but it doesn't seem 
> any cleaner.




https://reviews.llvm.org/D34304



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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.
>
>
> There is no other patch for using this preprocessor option. Any related 
> improvements down the road will be about general improvements for error 
> recovery, for example things like this:
>
> - Introduce an `UnresolvedTypename` type instead of changing unresolved types 
> to `int`.
> - For `@interace A : B`, don't completely drop `B` from the super-class list 
> of `A` if it is unresolved. These kind of improvements are not conditional.


That's great! I was always hoping we'd get some diag improvements out of these 
:D


https://reviews.llvm.org/D34263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 CompilerInvocation &Invocation,
+  vfs::FileSystem *VFS,
+  StringRef FilePath) {

ilya-biryukov wrote:
> klimek wrote:
> > If this indeed needs to return a possibly owned buffer, explain when 
> > exactly it is owned and when it is unowned.
> Done. It's owned when read from disk and not owned when taken from 
> CompilerInvocation.PPOptions' file-to-buffer remappings.
After some in person discussion, the idea now is to just always copy and return 
a unique_ptr


https://reviews.llvm.org/D34287



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 the things above this class, can we move those 
> > into an extra header?
> Do you have any suggestions of where to put it and how to call it?
> I didn't think it's a good idea to put something like 'PreambleFileHash.h' 
> and 'TempPCHFile.h' into 'include/clang/Frontend/'. (Given that they are 
> essential an implementation detail of PrecompiledPreamble, exposing them in 
> public include folders seems like a bad idea).
TempPCHFile looks like something we just want to put into the .cc file and 
store as a unique_ptr.
PreambleFileHash seems fine as an extra header.



Comment at: include/clang/Frontend/PrecompiledPreamble.h:157-160
+  /// We don't expose PCHFile to avoid changing interface when we'll add an
+  /// in-memory PCH, so we declare this function as friend so that it has 
access
+  /// to PCHFile field.
+  friend void AddImplicitPreamble(CompilerInvocation &CI,

ilya-biryukov wrote:
> klimek wrote:
> > Why not make it a member instead?
> To keep BuildPreamble, CanReusePreamble and AddImplicitPreamble close to each 
> other.
> I.e. PrecompiledPreamble only stores data used by these functions.
> 
> We could make all of those members, of course. Do you think that would make 
> API better?
Generally, if these are closely coupled to implementation details of 
PrecompiledPreample, I think that coupling is strong enough to warrant making 
them members.
On the other hand, making some functions members, and others non-members, and 
putting them next to each other in the .cc file also would work.


https://reviews.llvm.org/D34287



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34329: [GSoC] Clang AST diffing

[Manuel Klimek via Phabricator via cfe-commits]

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 the class?
Usually folks read a source file top-down, so if the most relevant pieces of 
code are at the top, the source file gets easier to read and understand.

Thus:
1. try to keep clutter (like classes and typedefs not used by a user of the 
API) in a different header, or forward declare them and define them in the .cpp 
file
2. sort things into public first, then private in classes.


https://reviews.llvm.org/D34329



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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-commits

[PATCH] D34329: [GSoC] Clang AST diffing

[Manuel Klimek via Phabricator via cfe-commits]

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 structure as well and make 
> > `InvalidNodeId` a private member. I suggest the following interface for 
> > `NodeId`:
> > 
> > ```
> > struct NodeId {
> > private:
> >   static const int InvalidNodeId; 
> > public:
> >   int Id;
> > 
> >   NodeId() : Id(InvalidNodeId) { }
> >   NodeId(int Id) : Id(Id) { }
> >   
> >   bool isValid() const { return Id != InvalidNodeId; }
> >   bool isInvalid() const { return Id == InvalidNodeId; }
> > };
> > ```
> > 
> > 
> > This way you'll get rid of a couple of variable initializations that use 
> > `InvalidNodeId`. You also won't need to call the `memset` when creating the 
> > unique pointer array of `NodeId`s.
> Ok, I did it like this. 
> 
> Can I create a header file inside lib/Tooling/ASTDiff and include it from the 
> public interface? This would help reduce the clutter.
> 
> Instead of NodeId we could also just use references / pointer types. I don't 
> see any particularly good reason for choosing either one above the other.
> I guess indices make it more obvious how to compute the number of descendants 
> and such. On the other hand, when using reference types, there is less 
> boilerplate to write for loops.
No, but you can create a header ASTDiffInternal or somesuch next to the header 
in the public dir.


https://reviews.llvm.org/D34329



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 FunctionDecl *FD, StringRef 
CTUDir,
+   StringRef IndexName);

I'd just spell out Cross & s/CTUDIR/IndexDir/ perhaps? (if CTUDir is to contain 
the indices)
I think as this is the major API entry point, a comment that explains the 
meaning of the arguments and how to use them would be nice.




https://reviews.llvm.org/D34512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 problem, can 
> > you elaborate?
>
>
> This gets back to why the original patch plumbed the boolean all the way down 
> to newInvocation.
>
> newInvocation is the function that actually discards the dependency file 
> options--today unconditionally.


Correct, and it shouldn't.

> But there is code that calls newInvocation directly (ClangFuzzer is one), 
> without going through a higher-level API. So I can't adjust the arguments at 
> a higher level and still preserve the old behavior.

Can't the call sites that use it themselves fix the arguments themselves? We 
should be able to provide a convenience wrapper for those use cases, I'd guess 
they all look basically the same?

> Unfortunately, newInvocation's argument list type is incompatible with 
> ArgumentAdjusters, so something else will need to be done. What do you 
> recommend?

I'd have expected something like:

1. create ArgumentAdjuster that filters out deps file creation args
2. make that the default in the higher level APIs that already use 
ArgumentAdjuster
3. for each call site that uses the lower level API, look into what data they 
have (probably just argv from main?) and create some nice ArgumentAdjuster 
wrapper so they can use the default argument adjuster with a 1 line change


https://reviews.llvm.org/D34304



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 declaration is
+  /// found in the index the corresponding AST file will be loaded and the
+  /// definition of the function will be merged into the original AST using
+  /// the AST Importer. The declaration with the definition will be returned.
+  ///
+  /// Note that the AST files should also be in the \p CrossTUDir.

In the future we'll want to create an index interface around this (which will 
probably serve also what the refactoring integration would be based on), 
instead of piping files and directories into all classes.

Perhaps we can start this by already pulling out a class ProjectIndex or 
somesuch,  with methods like loadASTDefining(...)?



https://reviews.llvm.org/D34512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 function will be merged into the original AST using
+  /// the AST Importer. The declaration with the definition will be returned.
+  ///
+  /// Note that the AST files should also be in the \p CrossTUDir.

xazax.hun wrote:
> klimek wrote:
> > In the future we'll want to create an index interface around this (which 
> > will probably serve also what the refactoring integration would be based 
> > on), instead of piping files and directories into all classes.
> > 
> > Perhaps we can start this by already pulling out a class ProjectIndex or 
> > somesuch,  with methods like loadASTDefining(...)?
> > 
> While I do agree to have an interface for that would be really good, but 
> maybe it would be better to first review and accept this patch and after that 
> design the interface in a follow-up patch (so https://reviews.llvm.org/D30691 
> is not blocked). What do you think?  
I'm generally fine with that, and mainly looped in other folks to see whether 
anybody else has concerns.


https://reviews.llvm.org/D34512



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 believe there is a clear better way to do it that 
is also fundamentally more useful than adding extra knobs for every problem we 
encounter. Finally, these changes do not address a fundamental bug, and 
workarounds exist, so there is no large benefit to do this now in a 
non-principled way rather than wait for a more principled fix. Overall, I do 
understand that this is a trade-off, but I hope you understand that we 
sometimes have to make decisions.


https://reviews.llvm.org/D37813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 would just try to use the 
> > Preprocessor for this, or whether we'd want to only allow a small subset of 
> > actual macros, but the general idea would be the same: The 
> > UnwrappedLineParser would parse the macro at the expansion location A(X) 
> > into an unwrapped line, and then parse the expansion into a child line, 
> > with the tokens tha tare not in the argument of the call being marked as 
> > fixed (parent child might also be better inverted).
>
> (As a side-note, I want to stress out that we would actually need a 
> 'reversible' description to support the namespace case, to allow generating 
> the end-of-namespace comment)


I believe this will be needed for various reasons. The plan is to make this 
similar to how we format #ifdef trees, for which clang-format already has full 
support.

> Is there any work on that side, any timeline when this may be supported ?

Some initial design work has been done, and Krasimir said that he's interested. 
No timeline though :(


https://reviews.llvm.org/D37813



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 ||
+   Styles[i].Language == FormatStyle::LK_None)) {

LanguageFound can't be true here.



Comment at: lib/Format/Format.cpp:903-907
+  for (int i = Styles.size() - 1; i >= 0; --i) {
+if (Styles[i].Language != FormatStyle::LK_None) {
+  Style->AddLanguageStyle(Styles[i]);
 }
   }

This seems a bit backwards. I'd have expected us to build up a Style structure 
after parsing above, and then just copy that into the result in the end.


Repository:
  rC Clang

https://reviews.llvm.org/D41487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 (!LanguageFound) {
if (Styles.empty() || Styles[0].Language != FS::LK_None) {
  return make_error_code(PE::Unsuitable);
}
...
  }



Comment at: lib/Format/Format.cpp:936
+void FormatStyle::FormatStyleSet::Add(FormatStyle Style) {
+  Style.StyleSet.Styles.reset();
+  if (!Styles)

Should we rather check that a style that's added doesn't have its own Styles 
map? (I'd say you're not allowed to add a Style that was gotten into another 
StyleSet).


Repository:
  rC Clang

https://reviews.llvm.org/D41487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 as well:
> >  
> > http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax
>
>
> This and WebKit's style seem like compelling arguments to support this option.
>
> klimek, djasper: Do you have any objections against landing this?


Agreed.
Generally LG minus that I'd significantly reduce the number of test cases :)




Comment at: unittests/Format/FormatTest.cpp:6980
ExtraSpaces);
+
+  FormatStyle SpaceBeforeBrace = getLLVMStyle();

There are super many redundant test cases here - I don't think we need to test 
that brace init detection works here, again.
I think given the code change we basically need 2 tests:
one where the previous opens a scope, and one where it doesn't.


Repository:
  rC Clang

https://reviews.llvm.org/D46024



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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?


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

[Manuel Klimek via Phabricator via cfe-commits]

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;

Why is this not just if (FirstTabWidth <= Spaces) then?



Comment at: unittests/Format/FormatTest.cpp:9372
+  FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+  unsigned OldTabWidth = Alignment.TabWidth;
+  Alignment.UseTab = FormatStyle::UT_Always;

Instead of doing the save/restore dance, just put it at the end of a test or 
create a new test (or alternatively create a new style as copy and then change 
the settings on that).


Repository:
  rC Clang

https://reviews.llvm.org/D48259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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

[Manuel Klimek via Phabricator via cfe-commits]

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;

guigu wrote:
> klimek wrote:
> > Why is this not just if (FirstTabWidth <= Spaces) then?
> Actually, that was my first way of fixing it. 
> 
> ```
> if (FirstTabWidth <= Spaces && Spaces > 1) // to avoid converting a single 
> space to a tab
> ```
> 
> Doing so will produce a correct output but introduce what I thought could be 
> an unwanted change : whitespace less than **TabWidth** might get converted to 
> a tab.
> The comment above the test seems to indicate that this behavior was not 
> originally wanted, that's why I changed my mind. 
> 
> But after thinking it over, I think my fix attempt also introduce an unwanted 
> change. I misinterpreted the original goal of this option.
> I went back to the official documentation and it says
> 
> > Use tabs whenever we need to fill whitespace that spans at least from one 
> > tab stop to the next one
> 
> This is clearly not what this code is doing. I can land another patch for a 
> more appropriate fix but I wonder if this option should stay like this. 
> Aren't people expecting the formatting to use as much tab as possible with 
> UT_Always ?
> 
> Unless I'm mistaken, the following example (**TabWidth** of 4)
> 
> 
> ```
> int a = 42;
> int aa = 42;
> int aaa = 42;
> int  = 42;
> ```
> would become (following what's written in the documentation)
> 
> ```
> int a= 42;
> int aa   = 42;
> int aaa  = 42;
> int  = 42; 
> // only spaces since there's never enough whitespace to span a full tab stop
> ```
> And this is what I would expect:
> 
> ```
> int a  = 42; // int a\t = 42;
> int aa = 42; // int aa\t = 42;
> int aaa= 42; // int aaa\t = 42;
> int  = 42; // int  = 42;
> ```
The intent of the Always setting was to mirror what the Linux kernel wants, so 
whatever that is, we should probably follow it :)


Repository:
  rC Clang

https://reviews.llvm.org/D48259



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 value after the closing }; doesn't really make sense in 
> > this test, but usually these are in calls
> > f([]() { ... }, foo, bar).call(...)
> > 
> > b) make .as("") have paramters that go beyond the limit
> > 
> > c) add another chained call behind .as(""). 
> Hello, sorry for the late follow up on this! 
> 
> Indeed an interesting thing to test, and so I did, the patterns you describe 
> seems to give strange indentation as far back as release_36 and probably has 
> always done so. The case I'm testing here worked in release_40 but stopped 
> working somewhere before release_50
> 
> maybe fixing those other cases can be done in another patch
> 
> cheers,
> Anders
I meant: please add these tests :)


Repository:
  rC Clang

https://reviews.llvm.org/D45719



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 `ArgumentsAdjuster`s unfortunately causes a copy of the 
> original command line arguments. Not sure how important this factor is 
> compared to spinning up a compiler instance, just wanted to point it out.


From my point of view, it looks like this isn't enough to be a huge thing in 
tooling. That said, if we can find a nice abstraction to pull out, I'm 
generally in favor :) (I know, I know, I'm not a big help)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56841/new/

https://reviews.llvm.org/D56841



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Manuel Klimek via Phabricator via cfe-commits]

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-save *all* files  before build (some editors do 
> it automatically?) the new behavior is the right one, for people who only 
> save current file and rerun build on the console the old behavior is the 
> correct one.
>  It all boils down to the argument "I want to see same errors that running 
> the compiler would produce".
>
> @klimek, @arphaman, @simark, WDYT?


I'm in yet another camp: I carefully save when I have something that is correct 
enough syntax, so I only want errors from with changes from the exact file I'm 
editing and the rest of the files in saved state.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Manuel Klimek via Phabricator via cfe-commits]

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 
> (all?) shows diagnostics for edited files instead of saved one. And it's 
> unexpected that headers have 2 states - visible and saved (which cannot be 
> viewed in IDE at all). Looks like performance will be same like usage of 
> 'auto save after delay' feature in editor, if we make debounce delay 
> configurable, what do you think?

don't most IDEs show whether a file is saved or just modified?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Manuel Klimek via Phabricator via cfe-commits]

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 diagnostics for `A.cc` when `A.h` is edited
> > - we should, this seems more obvious & important than what we do with drafts
> > - this interacts badly with using draft state, as this patch proposes - 
> > there are too many edits
>
>
> FWIW, one of my "pain points" when using vim+clangd is:
>
> - Edit `A.h` in a buffer (and forget to save)
> - Switch to `A.cc` in another buffer
> - Realize that I forgot to save `A.h`
> - Go back to save `A.h`
> - Jump back to `A.cc` file.
>
>   I would be very happy if `A.cc` can see the unsaved `A.h` when I am editing 
> `A.cc`. I think the update should be relatively less frequent with this 
> approach, as people don't usually update multiple files at the same time.  
> Not sure if I want all files that depend on `A.h` to be updated when I edit 
> `A.h` though; it seems much more complicated and less important (to me).


I can see this is a problem when you forget to save. On the other hand, one 
case I encounter frequently enough is:
edit A.h, save
go to B.cc (that includes it), see errors that it causes (here and in other 
files)
go to A.h, fix error, save
go to B.cc (or a different file that includes A.h) and wanting to see the error 
gone


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 tell me what it does.
What I'd have expected is an abstraction like 'Workspace' (which I believe is a 
domain term in IDEs) that would:
-> be in some form coupled to 1 build system
-> provide a file system view on the workspace
-> provide the ability to watch for changed files

Then the BuildSystem would:
-> provide the ability to watch for changed inputs / commands
-> provide the ability to "prepare all inputs"
-> potentially provide the ability to get a build graph

If Integration wants to be that, it should:
- not extend GlobalCompilationDatabase
- not have getCompileCommand itself



Comment at: clangd/BuildSystemPlugin.h:31
+  /// The 'compile_commands.json' has a weight of 1.
+  virtual float weight() = 0;
+  /// Attempt to detect and load the build system that resides in \p Dir.

Do you actually foresee more than 1 being available in a single workspace? 
Feels a bit premature generalization to me :)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54952/new/

https://reviews.llvm.org/D54952



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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' is a bit of a weird name, as it doesn't tell me what it does.
> > What I'd have expected is an abstraction like 'Workspace' (which I believe 
> > is a domain term in IDEs) that would:
> > -> be in some form coupled to 1 build system
> > -> provide a file system view on the workspace
> > -> provide the ability to watch for changed files
> > 
> > Then the BuildSystem would:
> > -> provide the ability to watch for changed inputs / commands
> > -> provide the ability to "prepare all inputs"
> > -> potentially provide the ability to get a build graph
> > 
> > If Integration wants to be that, it should:
> > - not extend GlobalCompilationDatabase
> > - not have getCompileCommand itself
> > 'Integration' is a bit of a weird name, as it doesn't tell me what it does.
> It is, sorry about that. This is not final. I intended to put this into the 
> 'buildsystem' namespace, so it'd be used as 'buildsystem::Integration', which 
> would make more sense.
> 
> > What I'd have expected is an abstraction like 'Workspace' (which I believe 
> > is a domain term in IDEs) that would:
> > -> be in some form coupled to 1 build system
> > -> provide a file system view on the workspace
> > -> provide the ability to watch for changed files
> The abstraction you're looking for is called `BuildSystem` in this patch.
> It is decoupled from watching for files (this is handled by 
> FileSystemProvider), it can watch files internally but tells the users about 
> the changes in terms of source files that had their inputs changed.
> 
> > Then the BuildSystem would:
> > -> provide the ability to watch for changed inputs / commands
> > -> provide the ability to "prepare all inputs"
> This is exactly what `BuildSystem` does.
> 
> > -> potentially provide the ability to get a build graph
> I was thinking about this as well, but still not sure if this information is 
> better inferred by clangd when running the compiler, so decided to leave it 
> out until we have a use-case for that.
> 
> 
> > If Integration wants to be that, it should:
> > not extend GlobalCompilationDatabase
> > not have getCompileCommand itself
> There is a discrepancy between the needs of the build system implementations 
> and what clangd has at the protocol level.
> - At the protocol level, clangd operates on the files and it needs to provide 
> features on a per-file manner: get diagnostics for a file, run code 
> completion in a file, format a file, etc.
> - At the build system implementation level, we care about different things: 
> discovering which build system is used, watching for changes to important 
> files (`BUILD`, `CMakeLists.txt`, `compiler_commands.json`, etc.), reporting 
> those changes to the build system users, building the generated files, 
> providing compile commands, adding build depenndecies, etc.
> 
> `buildsystem::Integration` serves as a layer that ties those two together and 
> provides an interface that higher layers of clangd are more comfortable with, 
> e.g. a file-based interface that `GlobalCompilatioDatabase` provides.
> 
> My initial goal was to **replace** the `GlobalCompilationDatabase` with 
> `Integration` and make it a non-abstract class, making it clear it just wraps 
> build system integration into a form that's a bit more convenient for clangd 
> uses. This turned out to be some work (we have `OverlayCDB` and test 
> compilation databases that rely on this interface, so it's some work and it's 
> not clear if that would make the code simpler at the end, so I decided to 
> postpone it for now.
> 
So, I would still vote for separating a Workspace from a BuildSystem, as I'd 
expect a Workspace over time to grow VCS capabilities, and I'd expect having 
one abstractions that covers both builds and VCSs confusing.

I now understand the "Integration" layer better, but it still looks like it's 
doing too many things:
specificially, it seems like it's basically at the same time a single view of 
all build systems / workspaces AND the thing that manages them - and I'd 
definitely split that up; that is, both of these are useful but they should be 
different classes.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54952/new/

https://reviews.llvm.org/D54952



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 CXXConstructorDecl 
then?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54309/new/

https://reviews.llvm.org/D54309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D33029#1423949 , @MyDeveloperDay 
wrote:

> In D33029#1423947 , @lebedev.ri 
> wrote:
>
> > In D33029#1423944 , 
> > @MyDeveloperDay wrote:
> >
> > > Adding the unit tests lets us see how this option will work in various 
> > > cases, it will let us understand that its not breaking anything else.
> > >
> > > I personally don't like to see revisions like this sit for 2 years with 
> > > nothing happening, I don't see anything wrong with this that would 
> > > prevent it going in so I don't understand whats blocking it?,
> > >
> > > if you had some tests and a release note I'd give it a LGTM (but as I've 
> > > said before I'm not the code owner, but someone wanting to address 
> > > defects and add capabilities. but I think we need to be able to move 
> > > forward, people will object soon enough if they don't like it.)
> > >
> > > Generally I don't understand why clang-format is so reluctant to change 
> > > anything, As such we don't have many people involved and getting anything 
> > > done (even defects) is extremely hard.
> > >
> > > It looks like you met the criteria, and reviewers have been given ample 
> > > opportunity to make an objection. the number of subscribers and like 
> > > tokens would suggest this is wanted,
> > >
> > > Please also add a line the in the release notes to say what you are 
> > > adding. In the absence of any other constructive input all we can do is 
> > > follow the guidance on doing a review, for what its worth I notice in the 
> > > rest of LLVM there seems to be a much larger amount of commits that go in 
> > > even without a review, I'm not sure what makes this area so strict, so 
> > > reluctant to change especailly when revisions do seem to be reviewed.
> >
> >
> > I don't have any stake here, but i just want to point out that no tool 
> > (including clang-format)
> >  will ever support all the things all the people out there will want it to 
> > support. But every
> >  new knob is not just a single knob, it needs to work well with all the 
> > other existing knobs
> >  (and all of the combination of knob params), and every new knob after that.
> >
> > It's a snowball effect. Things can (and likely will, unless there is at 
> > least a *very* strict testing/quality policy
> >  (which is 
> > https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
> >  about))
> >  get out of hand real quickly..
> >
> > Just 2c.
>
>
> Correct we should always be adding tests and show we don't break existing 
> tests... We need to apply the "Beyonce Rule".. "if you liked it you should 
> have put a test on it."
>
> We shouldn't just give up making improvements or fixing defects because 
> something is hard or complex.


The problem is that clang-format is already at a complexity level where most 
patches for new things we see are overly complex and will make adding new 
things *even harder*, reducing the ability to move at all.
As I've written before, I agree that we're in a bad state, and I'm truly sorry 
for it - we need to get active contributors into a state where they can make 
changes aligned with the architectural spirit of clang-format, which make 
clang-format at least not more complex :)

For this specific patch, yes, please change it as djasper suggested to be 
configurable in the BracketAlignmentStyle.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D33029/new/

https://reviews.llvm.org/D33029



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 looking though the parameter list is doesn't recognize a numeric 
> constant as being a valid thing it would see in an argument list and so falls 
> through the bottom to return false, it will only be where there is a 
> numeric_constant as the first argument, if it has a second argument and that 
> argument is just a simple type it would still fail, but switch them around 
> and it won't
>
> As I look more at this, even this would (and still would) fail to break
>
>   int TestFn(A=1)
>   {
> return a;
>   }
>
>
> originally I didn't have the counting of the <> but one of the unit tests 
> failed where we had
>
>   verifyFormat("LngType\n"
>"LngVariable(1);");
>   
>   
>
> I guess they wanted to do something like this when the type and name is very 
> long
>
> int
>  variable(1);
>
> but I think this gets thought of as being a potential FunctionDecl
>
> so I held off just adding tok::numeric_constant to the isOneOf(), and went 
> with the use case
>
> I think perhaps I could solve the default argument by adding a
>
> startsSequence(tok::equal, tok::numeric_constant)


How does this even work for

  void Test(A a)
  {
  }

This code seems to deeply assume that the { is the last thing in the line for a 
definition. One question is whether we can just mark this as a function 
definition when we see the { in the UnwrappedLineParser...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59309/new/

https://reviews.llvm.org/D59309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 merge rights - could someone commit this for me please?
>
>
> I'd actively encourage you to go get commit access, I think its much better 
> when the commit comes from the author.
>
> There just isn't enough people actively involved in clang-format (and other 
> tools) for us to get reviews even looked at (even for defects), we need more 
> people involved fixing defects and doing reviews, getting commit access shows 
> an intent to fix anything in that comes from your own review which is one of 
> the things the code owners push as being an objection to adding more code.


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 understand at all  how it matters. We cannot possibly support all 
style guides that come up with random variance. I do not think this option 
pulls its weight.


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-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 definitions in a similar manner, including 
> variables and struct members. So I don't see how a similar features which 
> aligns consecutive macros is something which doesn't make sense.
>
> Additionally, because the formatting will *undo* such formatting if done 
> manually, it means that the existing source code formatting this way cannot 
> be handled via clang-format. In my case, it essentially means that I cannot 
> use clang-format to enforce the style guidelines, as the macros definitions 
> are aligned.
>
> Additionally, aligning a series of macros makes sense because it helps 
> improve readability, and is thus something that I'd rather not lose if I were 
> to switch to using clang-format without the feature.







Comment at: lib/Format/WhitespaceManager.cpp:436
+static void
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+  F &&Matches,

I don't think the 'All' postfix in the name is helpful. What are you trying to 
say with that name?



Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+  F &&Matches,
+  SmallVector &Changes) {

Why an rvalue ref? Also, this is used only once, so why make this a template?



Comment at: lib/Format/WhitespaceManager.cpp:442
+
+  for (unsigned i = Start; i != End; ++i) {
+if (Changes[i].NewlinesBefore > 0) {

llvm style: use an upper case I for index vars.



Comment at: lib/Format/WhitespaceManager.cpp:473
+
+  // Line number of the start and the end of the current token sequence.
+  unsigned StartOfSequence = 0;

These are indices into Matches, not line numbers, right?



Comment at: lib/Format/WhitespaceManager.cpp:497
+
+  unsigned i = 0;
+  for (unsigned e = Changes.size(); i != e; ++i) {

llvm style: use upper-case I and E.



Comment at: lib/Format/WhitespaceManager.cpp:500
+if (Changes[i].NewlinesBefore != 0) {
+  EndOfSequence = i;
+  // If there is a blank line, or if the last line didn't contain any

Why set EndOfSequence outside the if below?



Comment at: lib/Format/WhitespaceManager.cpp:512
+
+// If there is more than one matching token per line, end the sequence.
+if (FoundMatchOnLine)

What's the reason for this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D28462/new/

https://reviews.llvm.org/D28462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 SINCE LAST ACTION
  https://reviews.llvm.org/D59309/new/

https://reviews.llvm.org/D59309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 approach, 
except I'd turn it around:
I'd only short-circuit out of the loop returning false in line 2064 if 
isLiteral() && (TemplateOpenerLevel > 0).
Alternatively, we could try to skip things within template <> - don't we have 
the closing > for an opening < in MatchingParen?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59309/new/

https://reviews.llvm.org/D59309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 no response.
>
> Was there anything you think people were objecting too other than the normal 
> "its a high bar to get in" and "its complex in here"?
>
> I think its fairer to the person submitting a revision if they don't want 
> feature in, we should give feedback as to why, but are we to assume silence 
> is acceptance? how long do we wait? who is the decision maker?
>
> I've personally never met an Engineer who didn't like having more knobs to 
> fiddle with so I don't believe the rational that having more options is bad 
> as long as they don't interfere with each other, for that there is the 
> "Beyonce rule", if adding an option breaks someone else then "if they liked 
> it they should have put a test on it!"
>
> As far as I can tell this LGTM (I'm not the code owner, just someone wanting 
> to help)
>
> In the meantime I have uploaded this patch to my fork, where I'm maintaining 
> a clang-format with revisions that seem ok, but get stalled in the review 
> process.
>
> https://github.com/mydeveloperday/clang-experimental


I think in this case there were no objections to the knob, but djasper had 
concerns about the code.
The problem in this case is that there was a 3 month pause between the review 
comment and the response, and for those intricate changes (and yes, 
clang-format's design is problematic here in that it makes these changes really 
hard to get right) it take a long time to think oneself into the problem to 
fully understand where it could go wrong.

Regarding the abstract argument brought up about tests - this is not about 
whether specific things break, but whether the code gets harder to maintain 
over time, which in the end will lead to nobody being able to make changes, 
regardless whether there is a reviewer or not - I think it's important we don't 
underestimate that risk & cost.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D40988/new/

https://reviews.llvm.org/D40988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 understand at all  how it matters. We cannot 
> > possibly support all style guides that come up with random variance. I do 
> > not think this option pulls its weight.
>
> So is this a personal opinion or based on a technical reason (other than the 
> normal mantra about risk & cost),


I don't think risk is what matters - in the end it's about cost, and cost is a 
very technical reason.

>   Could we give @reuk some positive feedback about what he needs to change in 
> this review to get this accepted? He has the other boxes checked about public 
> style etc..
>
> 
> It may not be your preference but its not without some merit for those of us 
> who like to fine tune our style, if that can be done with minimal impact to 
> other styles and is well covered by tests, then can we at least have some 
> discussion first

I'm fine having a discussion - my main question is why this matters. This seems 
a lot more arbitrary than other things we have.

> @reuk please lets continue to fix this revision so it builds as I want to 
> pull it into
> 
> https://github.com/mydeveloperday/clang-experimental
> 
> As an aside, Wouldn't it be great if JUCE was a public style so you could 
> just say
> 
> BasedOnStyle: JUCE
> 
> Where that style defines what JUCE uses? I've always found this odd, I've 
> never understood why we only have Google,WebKit,Mozilla,LLVM, I would have 
> thought it would have been nice to have other big guns Microsoft,Qt generally 
> its only going to be a single function setting the defaults.
> 
> I actually think a change like that would really help people so they don't 
> have to work out all the individual styles, it would help keep .clang-format 
> files small and neat and not full of a bunch of different options.
> 
> Even if JUCE was based on another style say "Google" the code should really 
> be saying
> 
> if (Style.isGoogleDerivedStyle())
> 
>   ..
>
> 
> and not
> 
> Style == FormatStyle::Google
> 
> When its specific to JUCE it would say
> 
> if (Style.isJUCEDerivedStyle())
> 
>   ..
>
> 
> I know when using LLVM, that being able to have a .clang-format file that 
> just says
> 
> BasedOnStyle: LLVM
> 
> is really nice, I don't have to go work out what the LLVM style is, I just 
> get what the project defines, I think it would be great that anything that 
> passes the public style test for submissions should really be able to add 
> that kind of configuration.




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-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 github 
> ). I've found this patch useful when 
> contributing to JUCE and I thought others might too.


Oh wow, wasn't aware how popular this is. Sigh. I'd like us to push them to 
change their style guide, but I guess that's not going to fly.


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-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 
> keep seeing this being said in reviews? but what is the cost? I assume you 
> don't mean a financial cost? could you define cost as a problem so we can 
> address it? what would it take to make `cost` not an issue?


The cost is financial, as it's developer time, which costs real money to 
companies. In the end, to support this, people like myself who are doing this 
as part of their job spend time that they'd otherwise spend to make other 
things better that might be more important. To make cost not an issue we'd need 
to change clang-format to make changes like this significantly simpler to 
review & maintain. I'll add a code comment in a moment, given that this is 
probably something that we'll allow given it fulfills the criteria we've set 
ourselves.


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-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,

I'm really confused about the changes os parens. It seems like this should just 
change the spaceRequiredBeforeParens(...), but instead seems to change parens 
in a way that completely changes the logic? (or alternatively is phabricator 
showing this wrong?)


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-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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/ :( Do you see 
any chance to fix this by extending the token merging? It's unlikely that the 
community will be convinced to add some minor parts of C# to clang proper just 
to support clang-format.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58404/new/

https://reviews.llvm.org/D58404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 unconditionally what we do, or decide against doing 
it.



Comment at: clang/include/clang/Format/Format.h:263-264
 SIS_WithoutElse,
-/// Always put short ifs on the same line if
-/// the else is not a compound statement or not.
+/// If Else statements have no braces don't put them
+/// on the same line.
+/// \code

This seems weird - why would we want this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59408/new/

https://reviews.llvm.org/D59408



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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' postfix in the name is helpful. What are you trying 
> > to say with that name?
> I'm not particularly fond of `All` either, suggestions welcome.
> 
> As the comment above explains, `All` refers to the fact that it operates on 
> all tokens, instead of being limited to certain cases like 
> `AlignTokenSequence` is. Maybe I should name //this// one 
> `AlignTokenSequence` and the other one `AlignTokenSequenceOuterScope`, or 
> something.
How about calling this AlignMacroSequence and the other one AlignMacros.
Comment here would be something like (as I don't think "scope" plays a role??)
// Aligns a sequence of macro definitions.

The problem is that I think the alignTokensAll don't make sense either as a 
function.
We should be able to inline this into alignConsecutiveMacros, and pull the 
std::function handed in either into its own function, or just define it as the 
first step in alignConsecutiveMacros.



Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+  F &&Matches,
+  SmallVector &Changes) {

VelocityRa wrote:
> klimek wrote:
> > Why an rvalue ref? Also, this is used only once, so why make this a 
> > template?
> It's an rvalue ref, because that's also the case for `AlignTokenSequence` 
> (wasn't sure why, so I left it as is).
> It's used only once, but the function is more generic that way, no? That's 
> the point of its generic name.
> Tell me if I should change it.
I think we need to look at this from first principles. We can fix the old code 
later if it doesn't make sense :)



Comment at: lib/Format/WhitespaceManager.cpp:442
+
+  for (unsigned i = Start; i != End; ++i) {
+if (Changes[i].NewlinesBefore > 0) {

VelocityRa wrote:
> klimek wrote:
> > llvm style: use an upper case I for index vars.
> Ok, I assume your style changed because this is copied from 
> `AlignTokenSequence`.
Yea, sorry, those are in violation (we should fix that at some point, but *not* 
in this patch :)




Comment at: lib/Format/WhitespaceManager.cpp:500
+if (Changes[i].NewlinesBefore != 0) {
+  EndOfSequence = i;
+  // If there is a blank line, or if the last line didn't contain any

VelocityRa wrote:
> klimek wrote:
> > Why set EndOfSequence outside the if below?
> It's from `AlignTokens`. I think it's because due to some of the loop logic, 
> it ends up not checking up to the point of the the last token.
> Without setting this and calling `AlignCurrentSequence()` once more at the 
> end, the last line of a macro group does not get properly aligned, the tests 
> fail.
I was suggesting to move it inside the if below, did you try that (sounds like 
you tried to remove it).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D28462/new/

https://reviews.llvm.org/D28462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 to not change anything, should we fall through here if 
(SLS_All)?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57687/new/

https://reviews.llvm.org/D57687



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,

reuk wrote:
> klimek wrote:
> > I'm really confused about the changes os parens. It seems like this should 
> > just change the spaceRequiredBeforeParens(...), but instead seems to change 
> > parens in a way that completely changes the logic? (or alternatively is 
> > phabricator showing this wrong?)
> I think this looks like a bigger change than it really is because I added an 
> open-parens on line 2548, which then affected the formatting of the following 
> lines. I also added the `isSimpleTypeSpecifier` check, so that 
> functional-style casts (`int (foo)`) are given the correct spacing.
a) why did you add the one in 2548? you didn't change any of the logic, right?
b) there are 3 extra closing parens in 2560, I see one more new opening in 
2556, but that also seems superfluous?
One question is whether we should pull this apart into bools with names that 
make sense :)


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-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 to combine all 3
+  Dollar->Tok.setKind(tok::string_literal);

Tiny nit: please try to keep comments formatted as sentence (upper-case first 
letter, "." in the end).



Comment at: clang/unittests/Format/FormatTestCSharp.cpp:19
+
+class FormatTestCSharp : public ::testing::Test {
+protected:

If everything's static, you don't need a test fixture at all, you can just make 
it plain functions and use TEST() instead of TEST_F().


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58404/new/

https://reviews.llvm.org/D58404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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) {

Ok, sorry, but you went one step further here than I imagined :)
The idea would be to unlinline this (and only this :D) and call it 
AligneMacroSequence.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D28462/new/

https://reviews.llvm.org/D28462



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 all the changes are based on what is really a 
> single clause that was already there before, namely
>
>   Style.SpaceBeforeParens == FormatStyle::SBPO_Always
>
>
> Now that clause is abstracted away to be (the abstraction in my view being a 
> nice part of this change, becuase it avoided duplication and made it easier 
> to reason about!)
>
>   Style.SpaceBeforeParens == FormatStyle::SBPO_Always  ||
>   (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses && 
> Right.ParameterCount > 0);
>
>
> i.e. always add a space, or add a space only when the number of parameters is 
> > 0 and I don't want a space on empty parameters. (simple enough!)
>
> So whilst I understand the arguments of code complexity, I feel it is 
> sometimes used to discourage, what is in at least my view seemingly 
> reasonable requests. (not my choice but reasonable all the same)
>
> This was why I asked what was meant by "cost", but you raise the other cost, 
> and that is one that is more legitimate... or is it?. have we seen it here as 
> a problem or do we just fear it?


This adds another condition to an already complex "if" that I think literally 
nobody fully understands, and I think it does so for very questionable value 
(why does it matter whether there is a space diff between the arg and no-arg 
case?).
Each change has a cost:

1. cost of implementing
2. cost of review & rework in review
3. cost of making the code harder to understand for further changes
4. cost of maintenance if the code is reworked, this feature needs to be kept 
working, thus slowing down new features

We *have* to offset this against the value of a feature, especially in a 
bikeshed coloring tool like clang-format.

> Having come from almost nothing to understanding a certain amount of of the 
> code recently in a relatively short space of time (enough to at least 
> contribute a few bug fixes), I'm confident that the "up to speed" costs are 
> actually currently relatively low, and as the Format Library is only 20,000 
> lines and Format.cpp being only ~2500, I don't consider that to be large 
> enough for the code to have come intractable already.

I am working on a patch (for over a year now :( to generalize macro 
configuration instead of having special cases that don't really work well, and 
it's much harder than it should be, as the architecture is getting in my way.

> Of course I understand when someone starts asking for an Option e.g.
> 
> `AllowSpaceAfterSecondCommaOfArgumentListButNotBeforeAnyOfTheOthers: true`
> 
> We may have gone too far ;-), but I almost think that perhaps those cases 
> might be more limited than we think in the end.
> 
> From my recent experiences one nod to complexity was from the C++ language 
> itself.. when trying to fix a bug in `isFunctionDeclarationName()` e,g,
> 
>   A foo(abc);
> 
> 
> does the mean a function foo() taking an argument abc and returning A or an 
> instance of class A called foo being constructed with an argument abc.. This 
> can have all the difference if you want to put a `BraceAfterReturnType`
> 
> The "cost" as you describe it here, over our ability to reason about the code 
> in my view is more likely to come from having to analyze snippets of 
> languages without the full parser to reason with.

Sure, that's where large part of the system complexity comes from, as tackling 
this is not easy.




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::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+ 

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

[Manuel Klimek via Phabricator via cfe-commits]

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-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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
 // identifier and template string.
 if (Left.is(tok::identifier) && Right.is(TT_TemplateString)) {

I don't understand this yet, which test broke? The TT_TemplateString looks like 
it's a JS thing?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59309/new/

https://reviews.llvm.org/D59309



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Manuel Klimek via Phabricator via cfe-commits]

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 here:
Basically, when we expand the range until nothing changes, do not only check 
whether the indent is correct, but also whether we'd change the spaces in the 
indent.
WDYT?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54881/new/

https://reviews.llvm.org/D54881



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits