[clang-format] Remove YAML hack to emit a BasedOnStyle comment

[jasilvanus]

When serializing a formatting style to YAML, we were emitting a comment # BasedOnStyle: <style> if the serialized formatting style matches one of the known styles. This is useful, but mis-uses the YAML API.

An upcoming change to fix keys with special characters by quoting them breaks this,
and will emit a non-comment key '# BasedOnStyle': <style> instead.
(#88763)

Thus, remove this hack.
Instead, we emit an ordinary key OrigBasedOnStyle that is ignored when reading back the data.
An alternative would be to just remove it completely.

Ideally, we'd emit a proper comment, but our YAML API doesn't support that.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Jannik Silvanus (jasilvanus)

Changes

When serializing a formatting style to YAML, we were emitting a comment # BasedOnStyle: <style> if the serialized formatting style matches one of the known styles. This is useful, but mis-uses the YAML API.

An upcoming change to fix keys with special characters by quoting them breaks this,
and will emit a non-comment key '# BasedOnStyle': <style> instead.
(#88763)

Thus, remove this hack.
Instead, we emit an ordinary key OrigBasedOnStyle that is ignored when reading back the data.
An alternative would be to just remove it completely.

Ideally, we'd emit a proper comment, but our YAML API doesn't support that.

---

Full diff: https://github.com/llvm/llvm-project/pull/89228.diff

1 Files Affected:

• (modified) clang/lib/Format/Format.cpp (+7-1)

diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index ccb2c9190e2eff..9b311343387ead 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -807,12 +807,18 @@ template <> struct MappingTraits<FormatStyle> {
         FormatStyle PredefinedStyle;
         if (getPredefinedStyle(StyleName, Style.Language, &PredefinedStyle) &&
             Style == PredefinedStyle) {
-          IO.mapOptional("# BasedOnStyle", StyleName);
+          // For convenience, emit the info which style this matches. However,
+          // setting BasedOnStyle will override all other keys when importing,
+          // so we set a helper key that is ignored when importing.
+          // Ideally, we'd want a YAML comment here, but that's not supported.
+          IO.mapOptional("OrigBasedOnStyle", StyleName);
           BasedOnStyle = StyleName;
           break;
         }
       }
     } else {
+      StringRef OrigBasedOnStyle; // ignored
+      IO.mapOptional("OrigBasedOnStyle", OrigBasedOnStyle);
       IO.mapOptional("BasedOnStyle", BasedOnStyle);
       if (!BasedOnStyle.empty()) {
         FormatStyle::LanguageKind OldLanguage = Style.Language;

[mydeveloperday]

This doesn't feel correct to me...

[jasilvanus]

This doesn't feel correct to me...

I'd also be fine with just removing the comment.

Edit: On second thought, I'd actually be more comfortable with removing the comment to prevent folks confused by the ignored extra key. But I'm not sure how important this info in the written YAML is, and whether it is relevant at all. Feedback by clang-format folks on that would be appreciated.

[HazardyKnusperkeks]

The question is, is there any real benefit of putting the commented style in the output?

[jasilvanus]

I've removed the dummy field and the comment now. This should prevent confusion.
I don't know whether anyone actually uses the emitted comment, there is no explanation,
and tests pass without it.

I suggest that if we later learn that there are valid uses that require the comment, we should add proper comment
support in the YAML writer, and add a test for it.

[mydeveloperday]

I'm ok with removing it if @owenca and @HazardyKnusperkeks are.

[HazardyKnusperkeks]

I don't see a use case for that comment, and thus I see no problem in removing it.