[PATCH] D82980: [NFC] Run clang-format on llvm-objcopy.

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In D82980#2125967 <https://reviews.llvm.org/D82980#2125967>, @MaskRay wrote:

> It is subjective but it seems to me that clang-format is making bad decisions for .td files. Many people don't format .td files (various lib/Target/*/*.td and include/clang/Driver/Options.td)


The td formatting is less robust, but works well for simple CLI option files. I'm not particularly tied to the format it chooses, but I do find the options files easier to read when consistently formatted.

The other td files you mentioned are much more complex, and I've tried using clang-format on them. It totally botches them and I don't think we should bother with complex files.

> Formatting C++ files is fine but it seems that we run into some discrepancy between clang-format versions. Formatting with latest clang-format is probably fine.

Yep

In D82980#2127279 <https://reviews.llvm.org/D82980#2127279>, @jhenderson wrote:

> No issue in principle with this, but we do need to figure out the canonical version of clang-format we want to use for this if we are going to do it. I have no personal opinion on it, but suspect my installed clang-format version is out-of-date, and that if I were to do the same thing you did I'd get different results.


What does `clang-format --version` look like on your machine? The one on my machine seems to be updated on a rolling basis (built from trunk on a regular basis), but `sudo apt install clang-format-10` is available to use a more stable version.



================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:210
   if (Split.second.getAsInteger(0, NewAlign))
-    return createStringError(errc::invalid_argument,
-                             "invalid alignment for --set-section-alignment: '%s'",
-                             Split.second.str().c_str());
+    return createStringError(
+        errc::invalid_argument,
----------------
jhenderson wrote:
> MaskRay wrote:
> > I suspect the difference is due to changed heuristics of clang-format (of different versions).
> Actually, I suspect it just wasn't formatted before - the "invalid argument for..." string below in the original is over the 80 character limit.
Yep, this is the reason why. FWIW I also tried an old version (clang-format-7) and it still formats this block.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:490
+  MatchStyle SymbolMatchStyle =
+      InputArgs.hasArg(OBJCOPY_regex)      ? MatchStyle::Regex
+      : InputArgs.hasArg(OBJCOPY_wildcard) ? MatchStyle::Wildcard
----------------
MyDeveloperDay wrote:
> MaskRay wrote:
> > Pre-merge checks may suggest that this is due to different versions of clang-format.
> > 
> > I wonder whether we want to format the block.
> I do believe there were some changes made recently in this area @krasimir, @Typz might like to comment
This is using clang-format from head (7d9518c8000bcd742b364a390bc79560f736dc96 at the time). Using `clang-format-10` restores it.

I agree the version of clang-format to use is an important choice. I'll revert these portions for now and punt the question for later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82980