OK, error, ambiguous consistency of comments in examples. #971

vgvassilev · 2016-11-09T00:58:31Z

Current stats:

git grep '// OK,' | wc -l
      60
git grep '// OK:' | wc -l
     246
git grep '// error,' | wc -l
      22
git grep '// error:' | wc -l
     257
git grep '// Error' | wc -l
       7

This PR introduces:

  // OK, -> // OK:
  // Error -> // error
  // error[,] -> // error:
  // ambiguous[,] -> ambiguous:
  // error[:] ambiguous[,]-> error: ambiguous:

If you consider this as an improvement please merge.

zygoloid

I think this needs more of a case-by-case approach. A colon makes sense when the following wording is the reason why the line is OK or an error, but does not make sense when we're saying both that it's OK (say) and something else about it. I've marked up some individual cases that are better without this change.

zygoloid · 2016-11-12T09:52:35Z

source/access.tex

@@ -840,15 +840,15 @@
  class Y;
  extern void b();
  class A {
-  friend class X;   // OK, but \tcode{X} is a local class, not \tcode{::X}
+  friend class X;   // OK: but \tcode{X} is a local class, not \tcode{::X}


This one should stay as a comma

zygoloid · 2016-11-12T09:52:43Z

source/access.tex

  friend void b();  // OK
  friend void c();  // error
  };
-  X* px;            // OK, but \tcode{::X} is found
-  Z* pz;            // error, no \tcode{Z} is found
+  X* px;            // OK: but \tcode{::X} is found


This one too.

zygoloid · 2016-11-12T09:52:59Z

source/basic.tex

@@ -945,7 +945,7 @@

 typedef int I;
 class D {
-  typedef I I;                      // error, even though no reordering involved
+  typedef I I;                      // error: even though no reordering involved


This one should stay as-is.

zygoloid · 2016-11-12T09:54:10Z

source/declarations.tex

@@ -905,7 +905,7 @@
 \begin{codeblock}
 constexpr int f(bool b)
  { return b ? throw 0 : 0; }               // OK
-constexpr int f() { return f(true); }       // ill-formed, no diagnostic required
+constexpr int f() { return f(true); }       // ill-formed: no diagnostic required


This one should stay as-is, and likewise for other "ill-formed, no diagnostic required" cases.

zygoloid · 2016-11-12T09:55:06Z

source/declarations.tex

@@ -1683,8 +1683,8 @@
 body.
 \begin{example}
 \begin{codeblock}
-auto  f() { } // OK, return type is \tcode{void}
-auto* g() { } // error, cannot deduce \tcode{auto*} from \tcode{void()}
+auto  f() { } // OK: return type is \tcode{void}


This one should stay as a comma. (We're not saying it's OK because the return type is void, we're saying both (a) it's OK, and (b) the return type is void.)

zygoloid · 2016-11-12T09:56:33Z

source/declarators.tex

@@ -620,7 +620,7 @@
 or allow it to be changed through a cv-unqualified pointer later, for example:

 \begin{codeblock}
-*ppc = &ci;         // OK, but would make \tcode{p} point to \tcode{ci} ...
+*ppc = &ci;         // OK: but would make \tcode{p} point to \tcode{ci} ...


This one should stay as a comma.

zygoloid · 2016-11-12T09:56:59Z

source/declarators.tex

@@ -2271,7 +2271,7 @@

 \begin{codeblock}
 struct onlydouble {
-  onlydouble() = delete;              // OK, but redundant
+  onlydouble() = delete;              // OK: but redundant


This one should be left alone.

zygoloid · 2016-11-12T09:57:25Z

source/declarators.tex

@@ -2706,7 +2706,7 @@
 \begin{codeblock}
  int f(bool b) {
    unsigned char c;
-    unsigned char d = c; // OK, \tcode{d} has an indeterminate value
+    unsigned char d = c; // OK: \tcode{d} has an indeterminate value


This one should probably be left alone, since it's really saying "OK, but d has an indeterminate value"

vgvassilev · 2016-11-12T16:38:53Z

That makes sense to me. Is the list of comments what should be reverted exhaustive?

tkoeppe · 2016-11-13T19:17:16Z

Here's some tangential guidance: We all agreed that error: is the right way to state errors, because the cause of the error is something objective. If you like, you could factor out a change that only makes the error comments consistent.

By contrast, "OK" doesn't generally have a natural explanation other than "it's OK because it follows the rules of the standard". Any additional explanation that you choose to call out is subjective. So we're actually fine with `OK, ..." in general, but we can discuss individual change suggestions. But expect that part of the change to take more work.

tkoeppe · 2016-11-17T23:11:02Z

@vgvassilev: ping - this PR needs rebasing.

vgvassilev · 2016-11-18T16:07:37Z

Rebased, addressing @zygoloid's comments.

vgvassilev · 2016-11-18T16:11:38Z

@tkoeppe thanks for the comments. Who is "we"? I am happy to remove the OK part of the PR if requested. I feel some of these substitutions are valid.

tkoeppe · 2016-11-26T19:39:54Z

@vgvassilev: Please rebase and rescan the draft for new occurrences of the style you're changing.

…ng out of a parallel algorithm terminates—but how? As directed by LWG Motion 21 from 2016-Isaquah.

…// Error.

…iguous?

Revert some of the mechanical * OK: back to OK, * ill-formed, no diagnostic required back to ill-formed, no diagnostic required * error: -> error, (in one case).

vgvassilev · 2016-11-28T13:09:18Z

@tkoeppe Done. It seems somebody did push -f and rewrote the history of the master, sigh...

There are a few occurrences of ill-formed; which I want to change to ill-formed: however I don't feel I should be piling more things on this PR.

tkoeppe · 2016-11-28T13:25:23Z

@vgvassilev: I'm sorry. We do that extremely rarely, and never after more than five or so minutes after a push. Very sorry if you got caught in a force-push.

tkoeppe · 2016-11-30T13:08:03Z

@zygoloid: I think you liked this PR in general. Could you review the updated situation?

jwakely · 2016-11-30T13:19:38Z

source/compatibility.tex

  base();
 };
 struct derived : base {};

-derived d1{};       // Error. The code was well-formed before.
+derived d1{};       // error: the code was well-formed before.


This change looks wrong.

Specifically, it's not an error because it was OK before. After the change the comment makes no sense. The comment is saying "This is an error but it was OK before" and the current text says that much better.

jwakely · 2016-11-30T13:35:05Z

I don't think any of the s/OK,/OK:/ changes are improvements. Most of the capitalization changes and removing trailing whitespace changes are fine. The s/error,/error:/ changes are generally OK in isolation, but when they occur in a group that contains both "error," and "OK," changing only the errors to use colons looks visually inconsistent (as stated above, the "OK" ones usually shouldn't have colons).

Overall, I think this is an attempt to impose an unnecessarily rigid rule on free-text comments, which don't need to be globally consistent to some fixed rule. Local consistency and clarity of meaning is probably more important. As the current patch actually changes meaning in some cases and decreases legibility in other cases, I think it would make the standard worse, not better.

Individual pieces of this change would be fine where they improve clarity, but I don't like trying to address every instance at once just in the name of consistency.

jensmaurer · 2021-11-03T07:58:05Z

See #5080 for further cleanup for "OK"

vgvassilev force-pushed the OK_error_consistency branch from bcaa114 to cd7bd3e Compare November 11, 2016 18:36

zygoloid reviewed Nov 12, 2016

View reviewed changes

tkoeppe force-pushed the master branch from d2a10b4 to c1f9b30 Compare November 17, 2016 14:36

vgvassilev force-pushed the OK_error_consistency branch from c3cc50b to d18f666 Compare November 18, 2016 16:07

tkoeppe force-pushed the master branch from bffe152 to a15786d Compare November 18, 2016 17:47

Dawn Perchik and others added 9 commits November 28, 2016 13:51

NB US-15, US-16, US-167, US-168, US-169, US-170, CA-17 P0502R0 Throwi…

4d68a64

…ng out of a parallel algorithm terminates—but how? As directed by LWG Motion 21 from 2016-Isaquah.

[util.smartptr.shared] Fix punctuation in synopsis comment.

90474c7

Prefer // OK: over // OK, ; // error: over // error, ; // error over …

e6d7513

…// Error.

Double space.

78d77ba

Prefer ambiguous: over ambiguous, ; error: ambiguous: over error: amb…

0610a1d

…iguous?

Trailing space.

095b304

// OK; -> // OK: // error; -> // error:

131071c

Rebase and address zygoloid's comments.

53a958b

Revert some of the mechanical * OK: back to OK, * ill-formed, no diagnostic required back to ill-formed, no diagnostic required * error: -> error, (in one case).

Rebase; Change Error: to error:

ebdef95

vgvassilev force-pushed the OK_error_consistency branch from d18f666 to ebdef95 Compare November 28, 2016 13:04

jwakely reviewed Nov 30, 2016

View reviewed changes

jwakely closed this Nov 30, 2016

jensmaurer mentioned this pull request Oct 14, 2021

P0847R7 Deducing this #5002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OK, error, ambiguous consistency of comments in examples. #971

OK, error, ambiguous consistency of comments in examples. #971

vgvassilev commented Nov 9, 2016

zygoloid left a comment

zygoloid Nov 12, 2016

zygoloid Nov 12, 2016

zygoloid Nov 12, 2016

zygoloid Nov 12, 2016

zygoloid Nov 12, 2016

zygoloid Nov 12, 2016

zygoloid Nov 12, 2016

zygoloid Nov 12, 2016

vgvassilev commented Nov 12, 2016

tkoeppe commented Nov 13, 2016

tkoeppe commented Nov 17, 2016

vgvassilev commented Nov 18, 2016

vgvassilev commented Nov 18, 2016

tkoeppe commented Nov 26, 2016

vgvassilev commented Nov 28, 2016

tkoeppe commented Nov 28, 2016

tkoeppe commented Nov 30, 2016

jwakely Nov 30, 2016

jwakely Nov 30, 2016

jwakely commented Nov 30, 2016

jensmaurer commented Nov 3, 2021

OK, error, ambiguous consistency of comments in examples. #971

OK, error, ambiguous consistency of comments in examples. #971

Conversation

vgvassilev commented Nov 9, 2016

zygoloid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgvassilev commented Nov 12, 2016

tkoeppe commented Nov 13, 2016

tkoeppe commented Nov 17, 2016

vgvassilev commented Nov 18, 2016

vgvassilev commented Nov 18, 2016

tkoeppe commented Nov 26, 2016

vgvassilev commented Nov 28, 2016

tkoeppe commented Nov 28, 2016

tkoeppe commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwakely commented Nov 30, 2016

jensmaurer commented Nov 3, 2021