[PATCH] D32298: [clang-format] Replace IncompleteFormat by a struct with Line

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.


================
Comment at: include/clang/Format/Format.h:1519
+  /// formatted due to a non-recoverable syntax error.
+  bool IncompleteFormat = false;
+
----------------
Maybe we should invert this and make this FormatComplete or something? Otherwise this is bound to lead to an unnecessary double-negative.


================
Comment at: include/clang/Format/Format.h:1522
+  /// \brief If ``IncompleteFormat`` is true, ``Line`` records a one-based line
+  /// number when a syntax error might have occurred. This is based on a
+  /// best-effort analysis, and could be imprecise.
----------------
s/when/at which/


================
Comment at: include/clang/Format/Format.h:1523
+  /// number when a syntax error might have occurred. This is based on a
+  /// best-effort analysis, and could be imprecise.
+  unsigned Line = 0;
----------------
no comma, I think.


================
Comment at: include/clang/Format/Format.h:1524
+  /// best-effort analysis, and could be imprecise.
+  unsigned Line = 0;
+};
----------------
Hm. Something we might need to be thinking about here is whether this should be the line before or after formatting. So specifically, if I format:

  int a
      = 1;
  f(

Then, this gets reformatted to:

  int a = 1;
  f(

Would we want the Line to be 2 or 3?


================
Comment at: lib/Format/Format.cpp:1886
+                               StringRef FileName,
+                               bool *IncompleteFormat) {
+  FormattingAttemptStatus Status;
----------------
nit: There shouldn't be a line break here.


================
Comment at: lib/Format/Format.cpp:1889
+  auto Result = reformat(Style, Code, Ranges, FileName, &Status);
+  if (Status.IncompleteFormat) *IncompleteFormat = true;
+  return Result;
----------------
In LLVM style, there should be a line break here.


https://reviews.llvm.org/D32298