hatcat / cg30_issues Goto Github PK

In the middle of page 230 it says:

For example, std::minmax will make use of std::min and std::max.

I looked at the Possible implementation at cppreference as well as libcxx, libstdc++ and MSVC's STL. Nowhere did std::minmax make use of std::min or std::max. Instead, the overloads taking a std::initializer_list make use of std::minmax_element.

Label: 4.5: T.1

Page 52: Short paragraph between code examples states "The call to release_handle should only be made if the handle was successfully gotten."
But on Page 53 "Using RAII" code example, the destructor calls release_handle unconditionally (line 5 of code example). There should be a check for the value of h:
~resource() { if (h != 0) release_handle(h); }

The phrase "if none is declared"

• should be "if none are declared"
• I have no idea what it refers to: none what?

Page 298 adds a specialization of std::default_delete for std::FILE. According to http://eel.is/c++draft/namespace.std this is not allowed as std::FILE is not a program-defined type.

Label: 5.6: E6

[From Arthur]
Page 167 Edit#1
Missing explicit on the constructor of int_wrapper. (ALL constructors should be explicit, except for copy and move constructors, and for constructors you consciously want to trigger implicit conversions from braced-init-lists, like some of the constructors of std::pair.)

In the last quarter of page 122 it says that std::auto_ptr was removed in C++14. But according to https://en.cppreference.com/w/cpp/memory/auto_ptr and https://eel.is/c++draft/diff.cpp14.depr it got removed in C++17.

Label: 3.1: I.11

Page 220

inline
"It signifies that the definition can be found in the translation unit".

No, it signifies that a definition is reachable from within a definition domain (there may be more than one in a TU) [basic.def.odr]/11. In other words: the compiler knows (or will learn) about the definition before the of the definition domain.
It also signifies that there may be more than one definition of an inline function or variable within a program (but different TUs) as long as they consist of the same token sequence [basic.def.odr]/13.3.

The inline keyword is - in general - a tool to prevent ODR violations and "just happens" to serve double duty as an optimizer hint. The only exception is within module purviews (which themselves are ODR violation preventers): there - and only there! - inline regains its original meaning as an optimizer hint.

Page 129

"Global objects" ?
There is no meaningful distinction between objects at different namespace scopes besides the namespace names alone, whatever namespace such an object happens to inhabitate. The global namespace isn't special in its behaviour other than being shared with C.

[From Arthur]
Page 18 Edit#1
...Interesting... that you should title this chapter "Bikeshedding is bad", and then devote it to bikeshedding. ;) Many of your bike sheds are the right color, e.g. nobody's likely to argue with "Use Standard C++"; but I very vocally believe you're on the wrong side with default arguments.

See e.g.
https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil

Your particular example is good, though. See e.g.
https://quuxplusone.github.io/blog/2020/10/11/overloading-considered-harmful/
https://youtu.be/OQgFEkgKx2s "When Should You Give Two Things the Same Name?"

In your make_office example, the different versions of make_office definitely shouldn't have the same name at all. There should either be a makeOffice, a makeTwoFloorOffice, and a makeNamedOffice; or else there should be just one version of makeOffice taking all four parameters all the time. These two approaches are isomorphic to the two approaches to openConnection I show in "When Should You Give..." around the 75-minute mark. Reusing the identifier make_office for both the bool-taking and string-taking versions is definitely a bad idea.

I assume we agree that both overloading and default-argument-ing are inferior to just using different identifiers for different functionalities. Titus Winters has a talk on API design where he talks about using overload sets only for functions that in some sense "do the same thing," which is clearly not the case for makeNamedOffice versus makeTwoFloorOffice. Titus would reserve overload sets for situations where it "doesn't matter" which overload is called, because they all "do the same thing"; e.g. you might have one overload taking (const char*), another taking (const string&), and another taking (string&&).

[From Arthur]
Page 168:
Missing parens in if n <= 1 return 1;

Page 42

[[nodiscard]] int fn(int a);

This does not declare an attribute, the function return object has an attribute specifier [dcl.attr]

First: "as remarked earlier". Please give exact reference. I'm kinda interested in the justification for using int types for money, but I don't remember where you said this and I don't feel like rereading 70 pages.

Secondly: int types for money? That's expressed in cents? How about the non-base-10 English money? I wouldn't make such sweeping statements. And even in the US system using int means expressing everything in cents? Don't banks use 1/10 of a cent?

Possibly related to #21

Page 24: 6 lines from the bottom you use first "default-initialized", and next sentence "initalized by default". Are those the same? Then I'd suggest using twice the same phrase. This is confusing.

Also what's the relation of the first sentence to the second? Right now I'm reading it as "If no constructor is defined, members are initialzied by default EXCEPT if they are of built-in type, then not." Correct?

Then page 26 line 3: "prior to the function .... " So: if there is no constructor, built-in type members are not initialized by default, but if there is a default constructor they are? Truly weird. Might be worth remarking on.

Page 42

template <class T, class U>
int fn(T a, U b);   // (1)

template <class T>
int fn(T a, int b); // (2)

template <>
int fn(float a, int b); // (3)

(2) is an overload, not a partial specialization!
(3) is a total specialization of (2)

[From Arthur]
Page 225 Edit#1
Inconsistent use of template vs. template.
Speaking of bikeshedding: I'm a strong partisan of template. :)

In your response to my Tweet here, you said that it should read "Defaulted parameters". Note that the title of this section should then read "Defaulted parameters can confuse matters in overloaded functions".

Sometimes the author refers to themself with first person singular and other times with first person plural.

[From Arthur]
Page 134 Edit#1
The Core Guidelines example is already a bug, because userspace code can't portably use library names like copy without qualification.

Example with UB: https://godbolt.org/z/qojq1nTE5

See e.g. https://quuxplusone.github.io/blog/2018/06/17/std-size/

[From Arthur]
Page 235 Edit#1
I skipped over the original "fopen example": you showed it using unique_ptr, right? unique_ptr is the right tool for the job in this case. As you say, unique_resource would be maybe-suited for resources that aren't pointers (aren't nullable). I think it would be better to show an example that actually wasn't a pointer and actually did require unique_resource. (Personally I doubt such examples exist, which is why unique_resource has never made it through WG21; but if you could come up with one, that'd be good, because FILE* definitely is not such an example.)

On page 231 it says, that Sean Parent gave a talk called "Code Seasoning". But when I take a look at cpp-seasoning.pdf, I guess the title was "C++ Seasoning".

Additionally, the link https://channel9.msdn.com/Events/GoingNative/2013/Cpp-Seasoning doesn't work for me. It redirects me to https://docs.microsoft.com/en-us/shows/. Maybe the footnote should mention a link to the YouTube-video instead?

Label: 4.5: T.1

In the code sample on page 299 and 300 there is a memory leak. While std::default_delete<DATABASE_PROXY>::operator() calls close_database(*p), it does not call delete p. I.e. it does not destroy the object of type DATABASE_PROXY that gets constructed on page 300 when calling new DATABASE_PROXY(....

Label: 5.6: E6

On page 153, 154, 155 and 156 occurs the code

-> decltype(t.get_commander)

But I assume it should be

-> decltype(t.get_commander())

Label: 3.4: ES.50

On Page 111, in the class template vector_f, there is a member function size(), which is defined as:

size_t const size(); // extract the size parameter

I assume that the const is at the wrong place and should be:

size_t size() const; // extract the size parameter

Label: 2.6: T.120

At the bottom of page 157 is following class:

class int_holder {
public:
  void increment() const { ++ m_i; }

private:
  int& m_i;
};

Page 158 starts with the sentence

This will not compile.

But in fact it does compile. See: https://godbolt.org/z/e4vo3xEMo

Label: 3.4: ES.50

Page 222

"If the object is of a class type ..."

The zero initialization part is correct. But if the class happens to have viable constructors, additional dynamic initialization takes place before main() which runs the constructor. Dynamic initialization may be folded into the compile-time constant initialization if the constructors in question are constexpr or consteval.

Page 42

extern "C" int x;

This is not a linkage declaration but a linkage specification [dcl.link].
This example declares an integer variable with name x that has external linkage and "C" language linkage [dcl.link]/8
These kinds of linkage are distinct.

The term "declaration" might lead to the idea that users can declare their own linkages.

Page 280

The definition of function f2() is incomplete. The first line in the function body should read return a; rather than return;

the following link has the complete code (with filling in the gaps) from use-template-metaprogramming-only-when-really-needed chapter.
Look at the comments where it says "book has typo" and similar.
If you have questions, I can point you to the exact issues.

https://github.com/kobi-ca/use-template-metaprogramming-only-when-really-needed

[From Arthur]
Page 140 Edit#1
using BizBuild = World::Buildings::Business;
should actually say
namespace BizBuild = World::Buildings::Business;
and it'd be useful to explicitly say "namespace alias," for the benefit of readers who are grepping for that phrase and wondering why you don't talk about namespace aliases at all. :)

Beauty is in the eye of the beholder.

I almost always use auto (and I have a hard time with my line manager who opposes it with nonsensical reasons): we often don't need to spell out the type of local variables returned by functions, especially when they would be insanely verbose, plus, as you briefly explain at page 248, automatic type deduction avoids the implicit conversions we would have if we initialised with the wrong type.

However, the usage of auto at page 62 and later seem all but beautiful to me :-)

auto options = std::ifstream(filename);
...
auto key = std::string{};
auto value = std::string{};

Really? It's longer than

std::ifstream options{filename};
std::string key;

and we're not gaining anything, apart from a vague consistency in style, or are we trying to mimic Rust?

I have a subscription to https://www.oreilly.com/ and reading the book on there (which I believe is in EPUB format). Basically, I am seeing words hyphenated when they shouldn't be:

So this is on iPhone:

The same page on a 4k monitor:

So, you can see some words are hyphenated when they overflow the line but it then (randomly) hyphenates words in the middle of the line which is odd.

I would imagine it's an issue with EPUB but it's jarring to my eyes to see things like com-pilers and con-structor

Page 72

The function int salary() in the middle of the page is declared with 5 parameters instead of 4! The 5-parameter version comes later in the text.

At the top of page 112 there is the function definition

size_t size() const {
  return static_cast<E const&>(*this).size; }

The parentheses after size and before ; are missing.

Label: 2.6: T120

Page 264

Near the end of the page, at the very end of the sentence, the c[2] is supposed to be p[2].

On Page 55 the destructor of RAII is defined as:

~RAII() { release_fn r; (t); }

I guess that instead it should be defined as:

~RAII() { release_fn r; r(t); }

Label: 1.6: NR.2

Before page 55 I thought it was to give a particularly concise statement of whatever point you were making.
However the box on page 55 "I have witnessed...." is completely pointless. I believe in print journalism it's called a "pull quote": a salient quote to whet the reader's appetite. IMO this has no place in a semi-serious book like this.

[From Arthur]
Edit#1 Page 15
"For example, in the finance sector the use of Boost is outlawed."
{{citation needed}}. I don't think you mean "outlawed" (which country's financial laws are we talking about, anyway?); I think you mean more like "frowned upon" or simply "uncommon".

typo in few locations using no_discard vs nodiscard
https://en.cppreference.com/w/cpp/language/attributes/nodiscard

I strongly disagree with the absolute blanket instruction not to use #pragma once.

I recognise that it is what the core guidelines say, but believe that it is advice built out of theoretical ideals, and not real-world, practical experience that could be help read-world users.

The current text says nothing about the things that can go wrong with include-guards, and hence the resultant time-saving benefits of #pragma once

Real-world experience

I am far from alone in saying this: I have spent way, way too much time tracking down build errors which turned out to be 2 or more headers having the same include guard.

Many people will only ever need to build on multiple compilers that already support pragma once... and are not writing code that will be released to other people.

To impose on them the time-costs of editing and debugging include-guards is a purist attitude that does not recognise the costs of the imposition.

Explaining the problem to readers

If this instruction to use include guards remains in the book, the section should start by actually giving two code snippets, one that uses pragma once, and one that uses include guards.

It should then demonstrate the hazards of include guards, e.g. via a scenario of copying an existing file to make a new class... but forgetting to update the include-guard name in the copy.

Then showing code which includes both headers and uses both types...

And showing the type of the compiler error that you get when the second header file is not read. Where the obvious thing to do is to assume you got the class name wrong in the second-loaded file, so you spend ages trying to work out what is wrong with the class you created - and if you are lucky, it will only take you a few minutes to realise that the problem was the name of the include guard.

But if you are new to this category of error, it may take you a long, long time to understand the problem.

Better (IMO) advice

I think the advice should be "if your source code is released to people and needs to run on a wide variety of compilers, you will have to use include-guards, and this is why the core guidelines advocate for include-guards.

However, if you know that the compilers your organisation is using currently all support #pragma once, then you and your colleagues will have a more efficient development process if you use #pragma once.

If your organisation changes and you later find that you need to convert to include-guards, then you can use a scripting language such as python to make the one-off conversion, at the time that you know you need it. And teach your colleagues to use the more-error-prone, but more-widely-supported, approach.

Alternative approach

Some projects that release code as single-headers use #pragma once in their separate headers, so they get the time-saving and error-saving in the development process...

And then their release process that combines the source code in to a single header strips out the #pragma once and inserts a single include-guard around the combined file.

Examples:

https://stackoverflow.com/a/62218074

I’ve been spending time reading the preface, intro and first chapter.

I’ve have several dozen comments to make, on just this section so far. Some are suggestions for wording - and some are fundamental things about the structure and nature of the content….

I’ve been doing it by marking up a copy of the PDF on my iPad.

My transferring my comments, 1-by-1, to GitHub issues - and having to identify the location of each comment - is going to be incredibly time-consuming…

And then you are going to need to go through my issues, and do the reverse… And for all I know, others have made some of the comments already - so it would be wasting my time to write them up and explain them.

Page 142,
auto t = Agg{};

Should be zero initialization since it is aggregate hence all will be zeros and deterministic.

Page 43

"Of course, it is uninitialized (unless it is declared at global namespace scope), ..."
This is independent of the global namespace but rather applies to all variable definitions at namespace scope, i.e. variables with static storage duration [basic.start.static]

int a;
namespace N1 {
    int b = 1;
    namespace N2 {
        int c = 2;
    }
}

compiles to

a:          .zero   4
N1::b:      .long   1
N1::N2::c:  .long   2

Page 287

The word 'optimize' does not have the Latin root 'optare'. It is rather derived from the superlative of the adjective 'bonus' (eng. 'good') of the climax

"bonus, major, optimus" (eng. "good, better, best").

Nevermind, we invoke optimizers to get the "optimus" (eng. optimal) lowering into machine code because we wish the best results from the compiler. And this is the second meaning of 'optare': to 'wish' something.

Sorry, I can't deny 9 years of intensive Latin education at school.

[From Arthur]
Page 8
Thanks for the shout-out!
"quuxplusone.github.io/blog" please (as going to just https://quuxplusone.github.io/ won't actually bring up the blog).

[From Arthur]
Page 137 Edit#1
If this were aimed at newbies, I'd caution against using the phrase "namespace Business" when what you mean is "namespace ::World::Buildings::Business". I like to compare namespaces to subdirectories; would you say that both "usr" and "local" are common ancestors of directory "bin" and directory "include", but directory "local" is the LCA? I hope not! That would be super confusing! (I often see students get confused about this, at least with placeholder names like "N1::N2".) Experienced programmers will understand what you actually mean, but then again, they might be just skimming anyway because they already know most of this stuff (or think they do).

I question the use of "Crushing" as the superlative of "Decisive"; I had to go back and look at the inheritance hierarchy to see that these were even different identifiers. :P I'd suggest maybe using something like log levels: Info, Warning, Error?

Your clamp example is a fun war story. But it still falls foul of https://quuxplusone.github.io/blog/2018/06/17/std-size/ Incidentally, your clamp also violated https://quuxplusone.github.io/blog/2021/05/05/its-clamping-time/ but that doesn't really matter because the "don't use common names and especially don't call them via ADL" rule is the more important.

On page 241 is following concept defined:

template <typename R, typename T , typename U>
concept sort_concept = std::equality_comparable<T, U>
and std::copy_constructible<T>
and std::incrementable<T>
and std::indirectly_readable<T>
and std::swappable<T>
and std::strict_weak_order<R, T, U>;

(Side note: The space in T , appears also in the book.)

Afterwards, following function is defined:

void sort(sort_concept auto begin, sort_concept auto end);

1. Regarding sort_concept:

• std::equality_comparable only has one template parameter. Did you intent to use std::equality_comparable_with<T, U> or maybe sentinel_for<T, U>?
• Shouldn't you use std::indirect_strict_weak_order<R, T, U> instead of std::strict_weak_order<R, T, U>?
• Why is it templated of both T and U? Wasn't there only one InIt on page 240 that we write this concept for?

3. The use of sort_concept auto in the definition of void sort(...) wont compile. The concept takes three arguments, but we only (implicitly) provide one argument.

Label: 4.6: T.10

"Recall the operation of a linker". I really dislike that "recall". You can only recall if the reader is guaranteed to know this, for instance if you've explained it in this book. Even then, I'd give a reference. But in this case: don't recall, just explain. Your one paragraph is fine.

Page 80: "the definition of std::string has changed. Except that that can not happen. Maybe you should make your example:

1. you write a very useful utitlity
2. other people build on top of it
3. now you break the ABI by changing the internals of your utility.

Oh wait, I just got to the bottom of page 83 and you say that GCC changed its implementation of std::string. But I thought C++ stressed ABI permanency? So what is that about then?

"What is an ABI"

Isn't it customary to use capitalization: Application Binary Interface, when you spell out an acronym? (Need to check with the local Chicago wielder.)

"The need for an ABI is one of the reasons" No, the existence of an ABI is the reason.

Bottom of page 81 "Recall the guideline". I do no such thing. I thought you were going to explain that in this chapter? How can I recall what I haven't learned yet?

And maybe you should spell out that guideline a little more explicitly? The last sentence if page 83 seems to sort of state it: "Calling interfaces should only use that minimal subset of types". But you never really spell it out. The verb "use" from the title is very vague and ambiguous.

I may be the 17th person to report this ... but there's a very tiny nit on p280 of the published book.

"If we rewrite this function while observing ES.5: "Keep scopes small," we might arrive at this:

int f2(int a, int b, float c) {
if (a > b) return;
prepare_for_invocation();
return my_special_type{a, b}.magical_invocation(c);
}

The sample code on has correctly got "return a;" on the first line of f2.
https://godbolt.org/z/cg30-ch5.4

At the end of page 111 there is

template <struct E> class vector_expression {

But struct is no valid type-parameter-key. It should be class instead:

template <class E> class vector_expression {

Label: 2.6: T.120

Page 153 "the public get_commander functions simple forward"

Have I mentioned how allergic I am to the word "simply" (or "just")? To me that's a signal that the writer is lazy, or even unclear on the topic themself. Since the latter is not the case, you reallyneed to waste more words on this.

First: do the work
So I thought I'd give the soldier a private: commander _c and let the work be return _c.

Hah. Your function is static and can not return non-static member. So why is your function static?

Then: undefined soldier::get_commander
Yes, I suspected that. What is the relation between your _impl function and the public functions?
I would love to understand and use this idiom, but your example is insufficient for that.

Please clarify. In fact, please do so right here and now because this issue unlike my others is not purely academic.