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