[PATCH] D46180: [clang-format] Refactor #include insertion/deletion functionality into a class.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: lib/Format/Format.cpp:1691
   // 0. Otherwise, returns the priority of the matching category or INT_MAX.
-  int getIncludePriority(StringRef IncludeName, bool CheckMainHeader) {
+  int getIncludePriority(StringRef IncludeName, bool CheckMainHeader) const {
     int Ret = INT_MAX;
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > I would personally keep the function non-const and not use mutable fields here.
> > > Even though it's logically const, I would strive towards keeping the things `const` only if there are actually immutable
> > > One particular problem that could be avoided is accidentally calling the const methods concurrently on different threads.
> > > 
> > > But up to you if you actually want to make this change.
> > Making this non-const would prevent us from making `HeaderIncludes::insert` const, which I do want to keep. Added a mutex to prevent race-condition... 
> Actually keeping things const and mentioning in the class comments that the class is not thread-safe because it uses regexes seems like a better choice.
> Most of the clients shouldn't pay for the mutex operations for the sake of the rare multi-threaded clients.
> WDYT?
Honestly, I'm a bit torn on this. Either approach is not perfect but either seems acceptable. Rolled back to const+mutable and added the comment.


================
Comment at: lib/Format/Format.cpp:1995
+  /// Inserts an #include directive of \p IncludeName into the code. If \p
+  /// IncludeName is quoted e.g. <...> or "...", it will be #included as is;
+  /// by default,  it is quoted with "".
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > Maybe make an API more explicit about the style of includes that should be used?
> > > Make it literally impossible to misuse the API:
> > > ```
> > > enum class IncludeStyle { Quoted, Angle };
> > > /// \p IncludeName is the include path to be inserted, \p Style specifies whether the included path should use quotes or angle brackets.
> > > Optional<Replacement> insert(StringRef IncludeName, IncludeStyle Style = IncludeStyle::Quoted) const;
> > > ```
> > > 
> > I think the risk is pretty low, so I'm not sure if it'd be worth spending another variable. From experience, the current callers would likely need to either carry another variable for each include or use the following pattern (as the IncludeName is often already quoted):
> > ```
> >  ... = insert(Include.trime("\"<>"), QuoteStyle);
> > ```
> I still don't feel encoding things in the strings is the right approach here.
> It raises some questions that we should handle/assert: what if users don't quote the strings at all? what is the leading and trailing quotes don't match? and so on and so forth.
> Being explicit about such things seems like a better choice.
> 
> > From experience, the current callers would likely need to either carry another variable
> It's not surprising, since the class is literally extracted from the middle of the include insertion implementation in clang-format.
Ok, I am convinced ;)


================
Comment at: lib/Format/Format.cpp:2016
+  /// be removed.
+  tooling::Replacements remove(llvm::StringRef Header) const;
 
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > Maybe also add a param of type `Optional<IncludeStyle>` and 
> > >   - only remove includes with the specified style, if specified the value is specified
> > >   - remove both styles the value is `llvm::None`.
> > Again, I'm not quite sure if it's worth another parameter for this.
> The parameter in this case seems to be even more useful, since we have two different behaviors for quoted vs unquoted strings.
> So it's really easy to misuse the API and make a mistake, thinking it does the right thing for you.
After offline discussion, I was also convinced here :)


================
Comment at: lib/Format/Format.cpp:2031
 
-  llvm::Regex IncludeRegex(IncludeRegexPattern);
-  llvm::Regex DefineRegex(R"(^[\t\ ]*#[\t\ ]*define[\t\ ]*[^\\]*$)");
-  SmallVector<StringRef, 4> Matches;
+  StringRef FileName;
+  StringRef Code;
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > What do we use the `FileName` for?
> > > On a higher level, it seems we shouldn't need it to implement what `HeaderIncludes` does.
> > `FileName` is needed to decide whether a header is a main header.
> Can we pass `bool IsMainHeader` instead? Can we not store it as a field? It seems to be only needed in the constructor, right?
This can be only calculated at `insert` time when `IncludeName` is provided - we can only tell if a new include is main header when both `FileName` and name of the new header are provided.


Repository:
  rC Clang

https://reviews.llvm.org/D46180