owenpan added inline comments.

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:435-454
+bool UnwrappedLineParser::precededByCommentOrPPDirective() const {
+  const size_t size = Lines.size();
+  if (size > 0 && Lines[size - 1].InPPDirective)
+    return true;
+#if 1
+  const unsigned Position = Tokens->getPosition();
+  if (Position == 0)
----------------
curdeius wrote:
> Please remove unused code.
Will do pending D116318.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:485
+        if (FormatTok->isNot(tok::r_brace) || StatementCount != 1 ||
+            IsPrecededByCommentOrPPDirective || // StartsWithBrace ||
+            precededByCommentOrPPDirective()) {
----------------
curdeius wrote:
> Please remove unused code.
Will do.


================
Comment at: clang/unittests/Format/FormatTest.cpp:23224
+
+  EXPECT_EQ("if (a)\n"
+            "  if (b)\n"
----------------
curdeius wrote:
> owenpan wrote:
> > owenpan wrote:
> > > MyDeveloperDay wrote:
> > > > any reason these can't be verifyFormats?
> > > Did you mean to add the expected part as a separate case? I don't think 
> > > it would add any value if there are no braces to remove in the first 
> > > place?
> > > Did you mean to add the expected part as a separate case? I don't think 
> > > it would add any value if there are no braces to remove in the first 
> > > place?
> > 
> > Nvm. I think you wanted something like `verifyFormat(PostformatCode, 
> > PreformatCode, Style)`? Yes, I could do that, but I would have to relax the 
> > restriction to calling `BracesRemover()` in Format.cpp, i.e. checking 
> > `isCpp()` instead.
> :+1: you should be able to use the 2-argument version of `verifyFormat`, no?
Will replace all `EXPECT_EQ`s with `verifyFormat`s. Question: when is the 
former preferred to the latter? It seems `EXPECT_EQ` is frequently used in 
FormatTest.cpp. 


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

https://reviews.llvm.org/D116316

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

Reply via email to