isocpp / isocpp/CppCoreGuidelines
"Lifetime Safety" paper copy editing comments
Open
@hsutter is already working on this.
Since Sep 26, 2016.
open
- Dominant language
- CSS
- Stars
- 45.3k
- Forks
- 5.6k
- PR merge metrics
- No merged PRs in 30d
Description
I don't know if you consider the stuff in docs/ to be "officially" part of the repo... but in case you do I have some copy-editing comments for the Lifetime Safety paper; there's also one substantive question I have about one of the rules. Feel free to do whatever.
Substantive question/comment
p17 says "do not throw a Pointer with lifetime other than static." If I am understanding these rules correctly and read them as-written, then:
- A std::string contains a const char* (or moral equivalent) to some heap block, so it is a Pointer
- Most/all standard exception types contain a std::string (or perhaps a const char*), so they are all Pointers
- I should thus not throw any standard exception types, or anything else containing a dynamically-generated or dynamically-stored string, except perhaps by saying
static std::runtime_error the_exception; throw the_exception;
Am I missing something?
Wording suggestions
- p8. Second "note" sidebar, "The only way to obtain an
owner<>is from new" could maybe continue with "or by transferring ownership from anotherowner<>." (Or maybe "moving" instead of transferring?) - p10. Very near top. "whose lset depends on" (2x). I found "depends on" to be confusing wording. Maybe "contains" and "has a non-empty intersection with" for the two occurrences, respectively? Assuming I'm understanding it well enough anyway.
- General statement; p19 7.2 is the first time it comes up. I think it's worth an explicit mention that (assuming I'm even supposing correctly here) that
[[lifetime]]annotations are not really overriding the rules in the same way that, say,[[suppress]]would be. Maybe something like "[[lifetime]]attributes do not imply that the analyzer is leaving code unchecked; code that uses[[lifetime]]is still fully-checked on both sides of the interface. The purpose of[[lifetime]]is to provide information to the analyzer that would be impractical or impossible for it to gather on its own, and allow it to efficiently check each function in isolation rather than your entire program at once."
Errors, assuming I'm understanding things correctly
- p8. The nittiest of nitpicks -- in the three "notes" bullet points 1/4 down the page, (i)
(anything, invalid) == invalidshould probably be{anything, invalid} == {invalid}for consistency, and (ii) there is a black instead of blue brace in the first bullet atlset(p) == {a, null} - p8. Example 1.1: I think
int i = 0; // lset(i) = {i}should belset(i) = {} - p9. Example 1.3:
int* a[100]; int* p = a[0]; // lset(p) = {a}should be something else; presumablyint** p = &a[0];to match the lset comment. - p16. The second of four bullets near the start of section 5: "A branch of the form
if(a || b) {...}is treated asif (a) { if(b) {} else {...} }" should be... something else. This was reported as #101 by vladtepesch; (s)he suggestedif (a) {...} else if (b) {...}" - p22. Example 8.3. "See examples later in the paper, notably 8.2 and 10", concerning
[[lifetime(lset)]]. However, I cannot discern much a connection to 8.2 (which is also earlier), though perhaps it's intended. - p24. Example 8.9, first code snippet.
delete buf;I think should bedelete buf.data(); - p25. Example 9.
owning<int*>should beowner<int*>.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.