Page MenuHomePhabricator

[clang-format] Add options to AllowShortIfStatementsOnASingleLine to apply to "else if" and "else".
ClosedPublic

Authored by curdeius on Sun, Apr 18, 8:53 AM.

Diff Detail

Event Timeline

curdeius requested review of this revision.Sun, Apr 18, 8:53 AM
curdeius created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSun, Apr 18, 8:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
curdeius edited the summary of this revision. (Show Details)Sun, Apr 18, 11:15 AM

How is

if (a) return;
else
  return;

formatted with the different options?

And from the documentation I think it was intended that only if is short, never the else. So I think it behaves correctly, nevertheless I think it should be possible, but I don't know wether with new enumerators or a new option.

How is

if (a) return;
else
  return;

formatted with the different options?

Do you have something specific in mind?

And from the documentation I think it was intended that only if is short, never the else.

There's already an option WithoutElse that should do exactly this.

How is

if (a) return;
else
  return;

formatted with the different options?

Do you have something specific in mind?

This is the example for WithoutElse from the documentation, and I don't think it is covered with the tests.

And from the documentation I think it was intended that only if is short, never the else.

There's already an option WithoutElse that should do exactly this.

Yeah, that one is either badly named, or the whole option is badly documented. :)

SIS_WithoutElse (in configuration: WithoutElse) Without else put short ifs on the same line only if the else is not a compound statement.

SIS_Always (in configuration: Always) Always put short ifs on the same line if the else is not a compound statement or not.

How I read the documentation the whole option is only about the if, the else is only checked if it is a compound statement or nor, it is not formatted.
As said I agree that one should also allow short elses.

Oh, you're right. Should we go for a separate option then? AllowShortElseStatementsOnASingleLine? What would control else if statements then?

Oh, you're right. Should we go for a separate option then? AllowShortElseStatementsOnASingleLine? What would control else if statements then?

I think I would prefer to expand this enum and deprecate WithoutElse, give them better names and documentation. But a different option would also be possible.

curdeius planned changes to this revision.Mon, Apr 19, 6:27 AM
curdeius added a subscriber: klimek.

More I look at the current state of this option, more I think it's misleading.
I would expect that the if after else is also controlled by this option. And, the last else as well (adding yet another option for that seems to be a bit an overkill)

@klimek, @MyDeveloperDay, I'd love to hear your input as well on the best course of action, as I've seen that you've discussed this topic a bit in https://reviews.llvm.org/D59087.

The current status quo looks like:

/// Different styles for handling short if statements.
enum ShortIfStyle : unsigned char {
  /// Never put short ifs on the same line.
  /// \code
  ///   if (a)
  ///     return;
  ///
  ///   if (b)
  ///     return;
  ///   else
  ///     return;
  ///
  ///   if (c)
  ///     return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_Never,
  /// Put short ifs on the same line only if there is no else statement.
  /// \code
  ///   if (a) return;
  ///
  ///   if (b)
  ///     return;
  ///   else
  ///     return;
  ///
  ///   if (c)
  ///     return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_WithoutElse,
  /// Always put short ifs on the same line.
  /// \code
  ///   if (a) return;
  ///
  ///   if (b) return;
  ///   else
  ///     return;
  ///
  ///   if (c) return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_Always,
};

I.e. WithoutElse does not depend on the else statement being compound or not. I think I'll push a NFC commit to fix documentation and add tests for this.

Anyway, what I'm inclined to do is to have these options:

  • Never (same as now)
  • WithoutElse (same as now, concerns only a simple if (c) f();)
  • OnlyFirstIf (renamed from Always, and Always kept for backward compatibility, it would behave as currently, so only the first if in the sequence of if else if else if else is concerned)
  • AllIfsAndElse (would do what I want to achieve in this patch, so format like this:
if (c) f();
else if (c) f();
else if (c) f();
else if();

Naming is hard. All suggestions are welcome :).

More I look at the current state of this option, more I think it's misleading.
I would expect that the if after else is also controlled by this option. And, the last else as well (adding yet another option for that seems to be a bit an overkill)

@klimek, @MyDeveloperDay, I'd love to hear your input as well on the best course of action, as I've seen that you've discussed this topic a bit in https://reviews.llvm.org/D59087.

The current status quo looks like:

/// Different styles for handling short if statements.
enum ShortIfStyle : unsigned char {
  /// Never put short ifs on the same line.
  /// \code
  ///   if (a)
  ///     return;
  ///
  ///   if (b)
  ///     return;
  ///   else
  ///     return;
  ///
  ///   if (c)
  ///     return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_Never,
  /// Put short ifs on the same line only if there is no else statement.
  /// \code
  ///   if (a) return;
  ///
  ///   if (b)
  ///     return;
  ///   else
  ///     return;
  ///
  ///   if (c)
  ///     return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_WithoutElse,
  /// Always put short ifs on the same line.
  /// \code
  ///   if (a) return;
  ///
  ///   if (b) return;
  ///   else
  ///     return;
  ///
  ///   if (c) return;
  ///   else {
  ///     return;
  ///   }
  /// \endcode
  SIS_Always,
};

I.e. WithoutElse does not depend on the else statement being compound or not. I think I'll push a NFC commit to fix documentation and add tests for this.

Anyway, what I'm inclined to do is to have these options:

  • Never (same as now)
  • WithoutElse (same as now, concerns only a simple if (c) f();)
  • OnlyFirstIf (renamed from Always, and Always kept for backward compatibility, it would behave as currently, so only the first if in the sequence of if else if else if else is concerned)
  • AllIfsAndElse (would do what I want to achieve in this patch, so format like this:
if (c) f();
else if (c) f();
else if (c) f();
else if();

Naming is hard. All suggestions are welcome :).

Sounds good.

MyDeveloperDay accepted this revision.Wed, Apr 21, 3:21 AM

this LGTM

curdeius updated this revision to Diff 342170.Sat, May 1, 1:13 PM
  • [clang-format] Apply AllowShortIfStatementsOnASingleLine: AllIfsAndElse to "else if" and "else". Keep backward-compatibility.
This revision is now accepted and ready to land.Sat, May 1, 1:13 PM
curdeius retitled this revision from [clang-format] Correctly apply AllowShortIfStatementsOnASingleLine: Always to else branch. to [clang-format] Add options to AllowShortIfStatementsOnASingleLine to apply to "else if" and "else"..Sat, May 1, 1:14 PM

I went a different way in order to keep backward compatibility.
I renamed Alwayss to OnlyFirstIf and added AllIfsAndElse that behaves as Always in my initial patch.
@MyDeveloperDay, please have another look.

Names of the options can certainly be improved, but I'm currently short on ideas. Any help is welcome.

curdeius updated this revision to Diff 342338.Mon, May 3, 2:13 AM
  • Fix format.

Still looks good to me.