[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/jasilvanus closed https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/owenca approved this pull request. https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/HazardyKnusperkeks approved this pull request. I don't see a use case for that comment, and thus I see no problem in removing it. https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
mydeveloperday wrote: I'm ok with removing it if @owenca and @HazardyKnusperkeks are. https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
Style == PredefinedStyle) {
- IO.mapOptional("# BasedOnStyle", StyleName);
jasilvanus wrote:
I don't know, but I'd prefer to avoid supporting it if it isn't even needed. We
can still add it if we want this back.
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
jasilvanus wrote: 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. https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
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);
jasilvanus wrote:
This allows the key being present in the input, avoiding an unknown key error.
But now with all of it removed, this is no longer relevant.
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/jasilvanus updated https://github.com/llvm/llvm-project/pull/89228 >From 5d4a3b0f922b0c28960f610c695c92da7d3538c1 Mon Sep 17 00:00:00 2001 From: Jannik Silvanus Date: Thu, 18 Apr 2024 14:56:47 +0200 Subject: [PATCH 1/2] [clang-format] Remove YAML hack to emit a BasedOnStyle comment When serializing a formatting style to YAML, we were emitting a comment `# BasedOnStyle:
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/HazardyKnusperkeks edited https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/HazardyKnusperkeks commented: The question is, is there any real benefit of putting the commented style in the output? https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
Style == PredefinedStyle) {
- IO.mapOptional("# BasedOnStyle", StyleName);
HazardyKnusperkeks wrote:
How hard would it be to add that?
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
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);
HazardyKnusperkeks wrote:
If we go that way, why read it back? This does not have any effect.
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
Style == PredefinedStyle) {
- IO.mapOptional("# BasedOnStyle", StyleName);
jasilvanus wrote:
I don't think our YAML writer supports comments at all.
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
jasilvanus wrote: > This doesn't feel correct to me... I'd also be fine with just removing the comment. https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
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);
jasilvanus wrote:
> I think clang-format will baff if there is styles in the .clang-format file
> we don't understand, its very unforgiving of new styles
This will set a key that is never read just for reference, as the old comment
did.
> Why is using # not a valid YAML comment?
Using `#` in the textual yaml is a valid comment and perfectly fine.
However, here we are using a function to set a key named `# BasedOnStyle`, which
just happened to emit a correct YAML comment because the key is not properly
quoted/escaped.
This breaks when fixing key quoting.
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
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);
mydeveloperday wrote:
I think clang-format will baff if there is styles in the .clang-format file we
don't understand, its very unforgiving of new styles
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
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);
mydeveloperday wrote:
Why is using # not a valid YAML comment?
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
@@ -807,12 +807,18 @@ template <> struct MappingTraits {
FormatStyle PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
Style == PredefinedStyle) {
- IO.mapOptional("# BasedOnStyle", StyleName);
mydeveloperday wrote:
is the a way to emit the a comment into the YAML at the correct indentation
level?
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/mydeveloperday requested changes to this pull request. This doesn't feel correct to me... https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/mydeveloperday edited https://github.com/llvm/llvm-project/pull/89228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
llvmbot wrote:
@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.
(https://github.com/llvm/llvm-project/pull/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
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 PredefinedStyle;
if (getPredefinedStyle(StyleName, Style.Language, ) &&
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;
``
https://github.com/llvm/llvm-project/pull/89228
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Remove YAML hack to emit a BasedOnStyle comment (PR #89228)
https://github.com/jasilvanus created https://github.com/llvm/llvm-project/pull/89228 When serializing a formatting style to YAML, we were emitting a comment `# BasedOnStyle:
