zwliew added inline comments.
================
Comment at: clang/lib/Format/Format.cpp:3393
if (!ChildFormatTextToApply.empty()) {
assert(ChildFormatTextToApply.size() == 1);
----------------
HazardyKnusperkeks wrote:
> zwliew wrote:
> > Is there a reason for this to be limited to 1? I came across this case
> > during testing, but I can't think of why this should be so.
> See comment on D93844.
I've added a new test case (9.1.2) to illustrate the test failure, and also
fixed the code to fix the test case.
================
Comment at: clang/lib/Format/Format.cpp:3276
+ // Check for explicit config filename
+ if (StyleName.startswith_insensitive("file:")) {
+ auto StyleNameFile = StyleName.substr(5);
----------------
zwliew wrote:
> HazardyKnusperkeks wrote:
> > zwliew wrote:
> > > Following my code suggestion above, I think this code block could be
> > > refactored to this:
> > > ```
> > > // Check for explicit config filename
> > > if (StyleName.startswith_insensitive("file:")) {
> > > auto StyleFileName = StyleName.substr(5);
> > > if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
> > > return make_string_error(ec.message());
> > > }
> > > LLVM_DEBUG(llvm::dbgs() << "Using configuration file " <<
> > > StyleFileName << '\n');
> > > return Style;
> > > }
> > > ```
> > >
> > > What do you think?
> > >
> > > Also, on another note, I feel like the `-style` option is too overloaded.
> > > Would it be better to use a separate option like `-style-file` instead?
> > You can certainly refactor code.
> >
> > What happens with `-style="Google" -style-file="Foo/Bar"` is it different
> > from `-style-file="Foo/Bar" -style="Google"`?
> > I do not perse disagree, but I think it makes stuff clearer if there is
> > only one option.
> Fair enough. Some ideas off the top of my head that resolve the ambiguity
> sound needlessly complicated. Thanks!
I've refactored the code in the latest revision (under
`loadAndParseConfigFile()`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72326/new/
https://reviews.llvm.org/D72326
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
