Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wholesale list of issues (with suggested fixes), as found in ISO14882:2020 #5428

Open
naiveforce opened this issue May 2, 2022 · 26 comments

Comments

@naiveforce
Copy link

The list is extensive, therefore best viewed against the source document. The file attached contains markups importable (e.g. using Foxit PDF Reader) against PDF version of the Standard's text.
isoiec_148822020.zip

Overall methodology of used markup primitives:

  • Replacements: contain text that should be substituted;
  • Inserts: contain text that should be added in the pointed out locations;
  • Strikeouts: candidates for removal;
  • Floating notes: editorial remarks that could not be easily attached to a specific text location;
  • Highlights: editorial remarks and "general thoughts" about specific text fragments.
    Aside from the specific replacement/additional text each of these can contain hints/comments delimited by square brackets.

Additionally, frequently reoccurring issues, that I feel are worth mentioning as a "preface", can be put into two general categories:

  1. Frequently occurring issues that have been pointed out:
  • using 0 in place of nullptr (e.g. in assignments and comparisons involving pointer types.);
  • using sizeof(expr) instead of the (grammar appropriate) sizeof expr;
  • redundant access specifiers (e.g. struct S {public:...}; class C {private:...}; struct D : public B {...}; etc.);
  • places exhibiting formatting issues such as text indentation, missing whitespace etc.
  1. Frequently occurring issues that have been left out:
  • using std::move, std::forward where move, forward would (according to context) perfectly suffice (can be easily fixed by searching/replacing using automation if need be.)

While these cannot be categorized as outright errors, I feel that fixing them would improve readability and general quality of the Standard's text.

@JohelEGP
Copy link
Contributor

JohelEGP commented May 2, 2022

The library part of the standard values consistency. Core wording is intentionally inconsistent in some regards.

using 0 in place of nullptr (e.g. in assignments and comparisons involving pointer types.);

Any 0 in the core wording is probably intentional. On the library side, it can only be pre-C++11 text. New additions do use nullptr.

using sizeof(expr) instead of the (grammar appropriate) sizeof expr;

The library uses the former.

redundant access specifiers (e.g. struct S {public:...}; class C {private:...}; struct D : public B {...}; etc.);

I think it was @tkoeppe whoe recently made a comment on how it should be on the library side.

using std::move, std::forward where move, forward would (according to context) perfectly suffice (can be easily fixed by searching/replacing using automation if need be.)

This is intentional, although I don't think that's explicitly stated anywhere like [contents] or https://github.com/cplusplus/draft/wiki/Specification-Style-Guidelines.

@jwakely
Copy link
Member

jwakely commented May 2, 2022

It would be much more helpful to provide plain text issues, ideally as github issues for each concern. I'm not going to install a closed source application to read what could be plain text. Based on Johel's comments (which are all correct), there might not actually be anything wrong here anyway.

@jensmaurer
Copy link
Member

I've added "Names from the standard library are shown without std:: prefix, unless needed for disambiguation. std::move and std::forward are always shown as qualified names."
to https://github.com/cplusplus/draft/wiki/Specification-Style-Guidelines

I can't read the .fdf file with the tools I have on my Linux box, it seems (besides "less", which yields rather non-useful processing instructions in ASCII). See @jwakely's remark.

@JohelEGP
Copy link
Contributor

JohelEGP commented May 2, 2022

Since this is based on ISO14882:2020, consider using what's marked as C++20 at https://github.com/timsong-cpp/cppwp#readme.

Based on Johel's comments (which are all correct), there might not actually be anything wrong here anyway.

I only commented on the points of #5428 (comment), the "frequently reoccurring issues".

I'm not going to install a closed source application to read what could be plain text.

I can't read the .fdf file with the tools I have on my Linux box

The same goes for me.

@jensmaurer
Copy link
Member

pdftk claims to handle FDF data, but it says the original PDF is not a form (which seems accurate):

$ pdftk htdocs/c++20.pdf fill_form isoiec_148822020.fdf output x.pdf flatten
Warning: input PDF is not an acroform, so its fields were not filled.

I can't find any annotations in the resulting PDF. (For example, there should be one on physical page 1430, numbered page 1411.)

@naiveforce
Copy link
Author

@jensmaurer
Thank you for your effort. I'm trying to find a way to convert it into something more digestible but it seems that ease of use and portability was never on top of design priorities for proprietary formats. And doing it manually is just unfeasible.

@naiveforce
Copy link
Author

naiveforce commented May 3, 2022

As for the rest and to sum things up from my perspective: I’m sorry to say that you're missing the point here, focusing on what I’ve explicitly listed as things that are not in error but aren't a good teaching material either. Fixing these can help people treating the document as a de-facto ultimate rulebook on latest coding standards, but is not necessarily required for the text to be correct.
But as always, the devil is in the details, and the myriad of little things, unlisted here for sanity’s sake, makes the text syntactically or semantically incorrect.
I do understand your reluctance to use tools that fail to meet your moral standards, but as this was literally the one and only tool, that could lead to more or less readable results, at my disposal while preparing this summary, you should understand my reluctance of putting myself through the ordeal of once again sifting through several hundred of items in search of things that might potentially pick your interest or more likely generate even harder to process output.
But if there's one brave or lucky enough to find a way of making use of my results, here's couple of additional pointers that could help:

  • You should be able to import the annotations file against 6th ed. (dated 2020-12) of the Standard’s PDF text;
  • From there you should be able to “summarize the comments” into for instance "document and comments with connector lines".

Best of luck and keep up the good work,
J.

@jensmaurer
Copy link
Member

@jwakely, it seems your edit has destroyed @naiveforce 's comment by interleaving your responses. Care to undo?

@jwakely
Copy link
Member

jwakely commented May 3, 2022

Argh! I must have clicked "Edit" not "Quote reply". I've restored it, and here's what I intended as a reply:

As for the rest and to sum things up from my perspective: I’m sorry to say that you're missing the point here, focusing on what I’ve explicitly listed as things that are not in error but aren't a good teaching material either.

We focused on the only bits we could read, which is what you wrote here, not the attachment.

Fixing these can help people treating the document as a de-facto ultimate rulebook on latest coding standards,

That is an explicit non-goal of the standard though. Those people should stop using it that way.

But as always, the devil is in the details, and the myriad of little things, unlisted here for sanity’s sake, makes the text syntactically or semantically incorrect.

Unfortunately, we have no idea what those issues are.

I do understand your reluctance to use tools that fail to meet your moral standards, but as this was literally the one and only tool, that could lead to more or less readable results, at my disposal while preparing this summary,

The rest of us manage to report issues without such a tool though.

  • You should be able to import the annotations file against 6th ed. (dated 2020-12) of the Standard’s PDF text;

This is also not terribly helpful, because there have been hundreds of changes to the draft since then. Some large, some small.

Ideally, we want reports of issues with the current draft, and it's even more useful to provide pull requests to fix the latex sources.

@jwakely
Copy link
Member

jwakely commented May 3, 2022

I've installed the Foxit reader, and apart from the linux version being very buggy, I can't figure out how to import the FDF file. The "Importing Comments Data" instructions at https://www.foxit.com/blog/using-comments-data-import-and-export/ don't work. The user manual goes to a download site with manuals for completely different versions of the reader (the linux download is version 2.4.4.0911 and there's no manual for that).

So it's not just about moral standards. You've just chosen a bad tool that excludes us.

@jwakely
Copy link
Member

jwakely commented May 3, 2022

The comment 'Not really an "operation"' appears to relate to [rand.dist.samp.plinear], presumably referring to UnaryOperation.

The comment 'Fix indentation' appears to relate to something in [temp.names].

Page 1370 'ios_base::'. Seems to refer to either the end of [locale.money.put.virtuals] or [locale.moneypunct.general], but I can't figure out what it's suggesting.

Page 1026 'This should be a public member or otherwise made accessible to the implementation of iterator<!Const>'. Presumably relates to an exposition-only member of join_view::iterator. This presumably refers to the inner_ and/or parent_ members. Making them public members would be completely wrong, and making them accessible isn't strictly needed because it's up to the implementation to make the effects of constexpr iterator(iterator<!Const> i) work as specified.

Page 1349: Remove redundant whitespace. Maybe refers to [locale.codecvt.members]?

Page 924 'Redundant'. Something in the <iterator> synopsis in [iterator.synopsis]. Maybe the public access specifier in the base-clause for the iterator tag types.

Page 1568 'Unmerge' - refers to something in [util.smartptr.atomic.weak]. Maybe the detailed descriptions of compare_exchange_weak and compare_exchange_strong.

Page 837 'ant [For consistency]'. Something in table 81 [tag:container.hash.req], but that table has been dismantled now. I think it's suggesting that "for const b" should be "for constant b".

Page 1556 'sizeof *this [Expression version of the syntax]'. No. NAD.

This is about 2% of the file, and many of the comments in the FDF are just processing instructions of some kind that I can't interpret by hand so I ignored them. I found four real issues that I've reported, but one was already fixed in the current draft. Extracting those was painful and tedious, I don't intend to do any more this way for now.

@JohelEGP
Copy link
Contributor

JohelEGP commented May 3, 2022

it's up to the implementation to make the effects of constexpr iterator(iterator<!Const> i) work as specified.

I have commented on this before. There's no wording to suggest that it "should just work". "Work as specified" can be interpreted as results in a compile-time error due to the use of the private members of unrelated classes. In this case, that seems mostly "sibling" instantiations, but I wouldn't dismiss the possibility of reaching into the sibling's parents also being part of it.

@jwakely
Copy link
Member

jwakely commented May 3, 2022

I disagree because this case is specified as prose. There is no sensible way to interpret "Initializes [...] parent_ with i.parent_" as "Initializes it with a compiler error." If it was a codeblock following "Effects: Equivalent to" then I might agree. But non-C++ prose cannot mean "compiler error" unless you intentionally choose the silliest possible reading of the text and then twist it a bit more for fun. Even for an "Effects: Equivalent to" codeblock, it's a hostile reading.

@jwakely
Copy link
Member

jwakely commented May 3, 2022

The point is that the standard tells you what the observable behaviour is, not how to implement that. If an implementor can't figure out how to meet the specification without being told "by the way, you might need to make this accessible" then they shouldn't be trying to implement the standard library.

Edit: Especially as the members that need to be accessible don't necessarily even exist. They're shown as private members in order to describe the observable effects. Normatively declaring a function as a friend just so it can access names that might not exist is letting the tail wag the dog.

@JohelEGP
Copy link
Contributor

JohelEGP commented May 3, 2022

That makes sense. And feels oddly familiar. Hopefully it sticks to me now.

@jwakely
Copy link
Member

jwakely commented May 4, 2022

Page 1489 'noexcept'. This probably refers to the options, depth etc accessors of recursive_directory_iterator, but that's missing intentionally, because they have a narrow contract as per [fs.class.rec.dir.itr.general].

Page 1015 'Format as exposition-only'. Something in [range.transform.iterator] but I don't know what.

Page 1324 'Should be made public or otherwise accessible (e.g. through appropriate friend declaration)'. Maybe refers to the exposition-only zone_ and tp_ members of zoned_time, which obviously should not be public.

Page 1472 'noexcept'. Maybe refers to path current_path(error_code& ec); or bool is_empty(const path& p, error_code& ec);. Both of those need to allocate memory, so can throw bad_alloc.

Page 1583 'For consistency'. No idea.

Page 116 'sizeof a (Expression version of the syntax)'. NAD.

Page 1523 '-> decltype(see below)'. Presumably refers to the operator<=> overloads for sub_match but I don't see any benefit to the change.

Page 1361 'Should prepend virtual to all declarations in this sub-clause to be in line with the enclosing entity declaration'. Presumably refers to the itemdecls in [locale.collate.virtuals].

Page 749 'args doesn't seem to be an argument pack in the above signatures.' Already fixed by 095b2c2 for LWG 3619.

Page 1561 'floating-point'. No idea.

Page 1693 'ios_base::'. Maybe refers to app in the (mode & app) == 0 expressions.

Page 405 'Redundant'. Probably refers to the public access specifier in the base clause in example 2. Core examples intentionally use an inconsistent mixture of styles specifically so nobody thinks it's a de-facto ultimate rulebook on latest coding standards".

Page 820 'X (see for instance Table 81)'. Pointing out that the "Return type" column of [tab:container.assoc.req] doesn't show X as the return type for the constructors, unlike [tab:container.unord.req]. No longer inconsistent since dismantling those tables.

Page 1499 'noexcept'. Referring to filesystem operations that need to allocate memory, so can throw bad_alloc, so can't be noexcept.

Page 670 'Redundant'. Refers to the public access specifier in the example in [util.smartptr.enab]. Examples are not intended to demonstrate recommended style.

Page 1486 'noexcept'. Presumably refers to the deref and increment members of directory_iterator. Those all have narrow contracts. Making them noexcept is not editorial.

Page 1010 'Redundant'. Presumably refers to the private: access in the iterator class synopsis.

Page 194 ", const double". Maybe refers to the final line of example 7, but it's intentional that it doesn't specify both types in the explicit template argument list. NAD.

Page 1290 'Should be made public or otherwise accessible (e.g. through appropriate friend declaration) to this function template.' Meh.

Page 534 'to'. Maybe suggesting s/a call of the/a call to the/ in [uncaught.exceptions] ?

Page 1561 'floating-point' again. Still no idea.

Page 91 ',' - missing comma in the list of cross-refs. Already fixed by 2878217

Page 1700 'std:: [For consistency]" - In example 1 wstring_convert is not qualified, but string and cout` are.

Page 1124: '2 [Base 2]' - ???

Page 384 'char' - not sure what this refers to, maybe that example 6 uses template <>, but that's intentional and fine. The standard is not a style guide.

Page 1378 'operated on' - ???

Page 1509 '->decltype(see below)' - as above re sub_match spaceship operators, but for the <regex> synopsis.

Page 1031 'This should be a public member or otherwise made accessible to the implementation of outer-iterator<!Const>.' - meh.

Page 318 '[Missing whitespace]' - maybe refers to the lack of space before the colon in A(): p(this) but it's fine.

Page 1366 'ios_base::' - maybe suggesting we say ios_base::goodbit rather than str.goodbit.

Page 248 '[fix indentation] - ???

Page 603 'and is_trivially_destructible_v<T> != true [See 20.6.3.3.]' - ??? Probably not editorial.

Page 948 'Redundant' - access specifiers for base classes of iterator tag types (again).

Page 1559 '[Format as code]' - ???

Page 1691 '_base' - suggesting to use ios_base::beg and ios_base::end instead of ios::beg and ios::end.

Page 392 '[Fix indentation.]' - ???

Page 1630 'calling' - ??? Maybe suggesting "as if by calling INVOKE" but that would be wrong, since it's not a function.

Page 1491 'noexcept' -- probably suggesting adding it to filesystem ops taking error_code, but they can throw bad_alloc so the suggestion is wrong.

Page 1017 'Redundant' - private access specifier in transform_view::sentinel. Meh.

Page 1332 'Missing whitespace' - ???

Page 1461 'noexcept' - filesystem ops again, wrong again.

Page 989 's' - ???

Page 143 'is' - ???

Page 447 '[Undefined class templates A and B.]' - refers to Note 5.

Page 1443 ' template' - ???

Page 1692 'from' - ???

Page 1548 '[Not really an "operation".]'

Page 1045 ',' - ???

@naiveforce
Copy link
Author

naiveforce commented May 4, 2022

Like stabbing in the dark ;(. Most of the annotations are based on visual cues, so yes distilling the fdf proved equally useless.
I've managed to find 2 "multi-platform" tools able to work with fdf files containing annotation data, both non-oss and offering equally useless Linux versions:

  • Adobe Reader -- 32-bit only; not able (in my case) to add/edit/import/export any annotations data;
  • Foxit -- can create new annotations, save them, view them but has no import/export capabilities under Linux.

If it helps, I've managed to reliably import fdf into pdf using Foxit on Android and iOS. But after hours of fruitless experiments, I'm starting to believe it's not really worth it.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented May 4, 2022

Page 1015 'Format as exposition-only'. Something in [range.transform.iterator] but I don't know what.

\tcode{iterator} should be \tcode{\exposid{iterator}} in para 1 and 2. (Fixed by #5441.)

@naiveforce
Copy link
Author

Found a way to redact the damn thing. But before I begin:

  • Will it be of any use at this point?
  • Will it be ok (copyright-wise) to post heavily redacted pages with comments only (~300)?

@jensmaurer
Copy link
Member

jensmaurer commented May 5, 2022

Thanks a lot! If you have a PDF that has (selected) pages with readable annotations, please send the PDF to me via e-mail first: jens.maurer@gmx.net . I'll share it on the committee-internal host and we can go from there.

@jensmaurer
Copy link
Member

I've uploaded PDF files with the annotations to the committee wiki pages (core and library) as c20-editorial-annotations-*.pdf .

@jwakely, I would suggest I handle the CWG comments by going through them, pushing whitespace and similar fixes right away, and (where necessary) creating editorial issues or pull requests for the more complicated things.

Once I've done that, I'll do the same with the LWG comments, unless you beat me to it.

@jensmaurer
Copy link
Member

jensmaurer commented May 5, 2022

I've looked at some the "fix indentation" concerns, and they are not present in my version of C++20 (but my version probably didn't go through ISO CS). Let's assume they're post-processing artifacts.

Same for some of the "whitespace" situations within sentences. Did you try a second PDF reader?

Suggestions to use sizeof ex instead of sizeof(ex) are summarily rejected.

@jensmaurer
Copy link
Member

[stmt.return] "The destructor for the result object is potentially invoked."

This is correct as written; the operand might not be an object at all (e.g. a braced-init-list) or might be converted to the result. If the operand creates a temporary, that's handled separately.

[dcl.fct] p22: "template<> void g1(const int*, const double&);"
This is correct as written; template arguments can be deduced in function template specializations.

"D contains declaration of A::B::C::i both directly through using namespace C and indirectly through using namespace B"
using-directives do not change the contents of namespaces, they just change the rules for lookup. The example seems to be correct as written.

@naiveforce
Copy link
Author

naiveforce commented May 5, 2022

@jensmaurer
Tried four I had at hand (namely Chrome, iBooks, Firefox and Foxit) all displaying consistent results for the places in question. Most probably it's a tool-chain problem, as the missing whitespaces usually occur together with typeface/formatting changes.

Thank you for clarification on things that really matter.

@jensmaurer
Copy link
Member

[class.union.general] "[How does this statement relate to the rest of the sentence (after first comma)?"

If you have a default member initializer for the first member of a union, it doesn't matter whether the default constructor for that member is trivial or not as far as the requirement to define your own union-level default constructor is concerned.

@jwakely
Copy link
Member

jwakely commented Aug 22, 2022

[uncaught.exceptions]:

- can result in a call of the function std::terminate
+ can result in a call to the function std::terminate

[tuple.cnstr] p14:

- Effects: Initializes the elements in the tuple with the corresponding value in std::forward<UTypes>(u).
+ Effects: Initializes the elements in the tuple with the corresponding value in std::forward<UTypes>(u)....

The example in [tuple.creation] p7 qualifies std::string but does not qualify tie, ignore or make_tuple. Arguably that makes sense because we're in the "scope" of <tuple> so those names are "local", but that rationale is weak and we should decide if that's what we want.


There are suggestions to make the destructor calls in [optional.assign], [optional.mod] etc. depend on whether is_trivially_destructible_v<T> is false, but that's unobservable and unnecessary. Calling a trivial destructor can be elided by the library implementation, or by the compiler, so there's no need to complicate the spec.


[variant.swap]:

- Preconditions: Lvalues of type Ti are swappable (16.4.4.3).
+ Preconditions: Lvalues of type Ti are swappable (16.4.4.3) for all i.

It's suggested that the assignment operators in [func.wrap.func.con] get their own subclause.


[meta.const.eval] p2 has an example where std::is_constant_evaluated() is qualified but memset isn't.


[char.traits.general]:

- a particular character container type (3.10) C, that and its related character traits class X are passed
+ a particular character container type (3.10) C, that C and its related character traits class X are passed

[ostreambuf.iterator.general]:

- writes successive characters onto the output stream from which it was constructed.
+ writes successive characters into the stream buffer for which it was constructed.

[alg.threeway], suggestion to add -> decltype(see below) to the second overload (and in the synopsis to match).


The changes above are the non-obvious ones from reviewing about half way through the c20-editorial-annotations-lib.pdf doc. I ignored some as harmless or unnecessary to change. I will submit a pull request for some obviously correct changes.

I'll review the second half of the doc soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants