Recommendations about coding in C++
software.intel.com
software.intel.com
static bool IsInterestingError(int errno) { ... }
...
if (!IsInterestingError(errno)) { ... }
I agree that the option (of passing the helper function no arguments at all) is not perfect either, but... (I also don't like that you need to call your stored errno's errnums or the like, but that's what we have.)
static bool IsInterestingError(int e) { ... }
Furthermore, C11 literally defines errno as a macro in 7.5p1:
The macros are
EDOM
EILSEQ
ERANGE
which expand to integer constant expressions with type int, distinct positive values, and
which are suitable for use in #if preprocessing directives; and
errno
which expands to a modifiable lvalue that has type int and thread local storage
duration, the value of which is set to a positive error number by several library functions.
If a macro definition is suppressed in order to access an actual object, or a program
defines an identifier with the name errno, the behavior is undefined.
While C11 is most definitely not the same thing as C++11, I think, similar to atomics, the committees cooperated on the threading API. I don't have a recent C++ standard handy at the moment, but I wouldn't be surprised if C++11 at least allowed defining errno as a macro.
Something's wrong with us if this has to be mentioned in a list like this.
If you want to convince a single other person that a C program you have written has no undefined behavior, then you and that person have to hopefully interpret English the same way. It would be completely hopeless to try to convince a hundred people that a nontrivial C program has no undefined behavior. And successful programs typically have way more than a hundred users.
static void takes_null_reference_by_default(int& p = *((int*) nullptr)) {
if (&p) ++p;
}
int main(int argc, char* argv[]) {
int v1 = 15;
int v2 = 25;
takes_null_reference_by_default(v1);
takes_null_reference_by_default();
takes_null_reference_by_default(*((int*) nullptr));
takes_null_reference_by_default(v2);
std::cout << v1 << " " << v2 << std::endl;
}
./a.out still runs though. And the de-references are just pseudo-de-references, it never "really" happens.
EDIT: also mentioning the fits "delete this;" gave some people, but worked very well in the ACE Networking Library and various other places.
And COM is future of Win32, rebranded as UWP.
Although I'm extreme anti-advertisement (with dedicated firewall to block web ads), I find this article having 3 qualities that's valuable:
1) instead of showing contrived bad code nobody writes, it shows real bugs from real projects such as MySQL, PostgreSQL, ReactOS, Apache server, Audacity, LibreOffice, Notepad++, CoreCLR, etc
2) full explanations of the bug and further advice for correcting the code
3) learning is free: you don't have to buy PVS-Studio to apply the solutions learned from the article or gain the ability to spot similar bugs later
That all adds up to a quality article which is better than the old PC-Lint ads[1] from Gimpel Software. For those too young to be aware of them, in the 1990s, GS ran ads in a magazine called C/C++ Users Journal. For the others that enjoyed the "puzzles" in those ads, Andrey Karpov's article is a fine continuation of that.
[1] example: http://www.polyweb.com/blog/index.php/archives/9
It would be way less error-prone to calculate the number of elements using sizeof, for instance. This would also make the function independent of the type (a template could be used), thus reusable in another contexts.
This is a strong statement I'm a bit wary of. I get the intent in context but it seems overly general.
BOOL WINAPI DllMain( HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved )
{
(void)hinstDLL; /* avoid warning */
(void)lpvReserved; /* avoid warning */
switch (fdwReason)
{
case DLL_PROCESS_ATTACH:
{
TCHAR szBuffer[64];
// This code will attach the process to its parent process
// if the parent process had set the environment variable.
// The corresponding code (setting the environment variable)
// is desktop/win32/source/officeloader.cxx
DWORD dwResult = GetEnvironmentVariable( "ATTACHED_PARENT_PROCESSID", szBuffer, sizeof(szBuffer) );
if ( dwResult && dwResult < sizeof(szBuffer) )
{
DWORD dwThreadId = 0;
DWORD_PTR dwParentProcessId = (DWORD_PTR)atol( szBuffer );
if ( dwParentProcessId && GetParentProcessId() == dwParentProcessId )
{
// No error check, it works or it does not
// Thread should only be started for headless mode, see desktop/win32/source/officeloader.cxx
CreateThread( NULL, 0, ParentMonitorThreadProc, (LPVOID)dwParentProcessId, 0, &dwThreadId );
// Note: calling CreateThread in DllMain is discouraged
// but this is only done in the headless mode and in
// that case no other threads should be running at startup
// when sal3.dll is loaded; also there is no
// synchronization with the spawned thread, so there
// does not appear to be a real risk of deadlock here
}
}
return TRUE;
}
case DLL_THREAD_ATTACH:
break;
case DLL_THREAD_DETACH:
osl_callThreadKeyCallbackOnThreadDetach( );
break;
}
return TRUE;
}
FWIW, he says to use static analyser tools to analyze code. We do that already, and extensively. Seems a bit provocative...
Appreciate the work you have done!
http://opengrok.libreoffice.org/diff/core/sal/osl/w32/dllent...
This is the 42 reasons why I give up C++.
> Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort.
The article states that these tips are 'usually universal', but I hardly found that to be true, only having counted about 10 points not being about C++.
The title should probably be something like the obvious "42 tips for C++ programmers" as the current one really doesn't justify the content.
I agree the title originally as submitted[1] is not helpful. When the author submitted[2] it to Intel DZ, he re-used the same vague title as the original PVS-Studio article from April 2016: http://www.viva64.com/en/b/0391/
HN admins already edit titles with [YEAR] to signify an old article. In the same vein, I do wish the admins (or the original submitters to save the admins the effort) went even further and exercised editorial judgement to add [NON-CUTE SUBTITLE] to negate the effects of cute-but-meaningless titles.
In this case, there would be a "[C/C++ refactoring tips]" like you suggested appended to the end of the title.
[1] original submitted HN title: "The Ultimate Question of Programming, Refactoring, and Everything"
[2] https://software.intel.com/en-us/articles/how-to-contribute-...
The linked article is incredibly detailed, much more so than 90%+ of articles that pass through HN. It talks about nuances, e.g. that copy-paste mistakes more than likely happen on the last line as opposed to any other line. Claiming it can be replaced with a two word phrase is wildly incorrect.
Having a tool that is trained to spot this kind of human errors is good. It can be useful in any language, not only C++. Avoiding them in the first place and giving you advices on how to not do it again would be a nice thing compiler could provide. i.e:
a[0] = b[0];
a[1] = b[1];
a[2] = b[0];
^ warning: Are you sure you want to assign b[0] to a[2]. If not, consider rewriting your code with a for loop
a[3] = b[3];It's surely worth using it.
Rust and Swift have sum types, so their equivalent of `memcmp` can have a return type with exactly three values (`Less`, `Equal`, `Greater`).
> 3. Copy once, check twice
I'd like to see the context of this code. Perhaps this could have been written more cleanly using iterators.
> 4. Beware of the ?: operator and enclose it in parentheses
Not a problem in Rust. `if cond { true_case } else { false_case }` is an expression in the obvious way.
> 5. Use available tools to analyze your code
The right tool is called “type checker”, but of course, the language has to be intelligently designed, rather than just evolved, for the type system to be useful.
> 6. Check all the fragments where a pointer is explicitly cast to integer types
Or, even better, don't do it at all.
> 7. Do not call the alloca() function inside loops
Or even better, don't do it at all.
> 8. Remember that an exception in the destructor is dangerous
Rust solves this neatly: use different tools for recoverable and unrecoverable errors (the `Result` sum type and panics, respectively). Destructors always return `()`, so they can't fail in a recoverable way.
> 9. Use the '\0' literal for the terminal null character
Or even better, use Rust-style iterators and forget this null-termination nonsense.
---
I could go on, but this is getting tiresome.
That's not what I'm saying. I'm showing how better-designed languages (which, to be fair to C++, have the benefit of over 20 years of hindsight) avoid some of the pitfalls you list.
And even if you added those features to C++, you couldn't guarantee a codebase that only uses the secure subset. Nobody has the time to rewrite old code. And making sure that current code uses only the secure subset would require code reviews. Not everyone does that.
While it's well structured, with real-world examples, it's very basic. I'm not an excellent C++ coder by any stretch of the imagination. I make lots of mistakes. But not these. The article is fairly thorough and long, so if you're experienced you'll invest non-trivial time for little or no reward.
We all make mistakes; and it's oftenworthwhile to review what we take for granted that we know.
I'd honestly welcome a post about different static analysis tooling that's libre and open source. Their pros and cons as well as how to deploy them would be good too.
Also this is about PVS-Studio, nothing to do with Microsoft, they don't own the Studio word.
People deserve to be paid for their work if it's useful.
Shouldn't they focus on examples of unexpected or non-obvious mistakes (or at least start with those -- but then why add obvious examples at the end)?
Whether you copy and paste or simply type a single line incorrectly, of course if the wrong thing is entered the code will simply be incorrect.
And to support both our arguments -- that example first mentions copy-pasting being the likely cause of the error and then provides a rewritten example that doesn't use the same code repeatedly (your point).
But a switch-case statement is a counter-example that you would copy-paste code and isn't abstracted into variable array indexes.
Counter-counter-argument: switch-cases have more obvious differences in a (only potentially) multi-character string.
No. Some people say they're a sign of a poorly architected object model.
1. both cases have identical code: just fall through from one to the next
case A:
case B:
// code for A and B
break;
case A:
// code for A only
case B:
// code for A and B
break;
case A:
// code for A only
goto AB;
case B:
// code for B only
AB:
// code for A and B
break;
switch(x) {
case A:
case B:
printf("value is: %d\n",important_array[x]);
if(x==A) /* something */;
} else if(x==B) /* something else */;Of-course these are obvious errors once you are given 10-15 lines of code and told that there is an error there. Imagine having this code in the middle of say, several million loc.
Anyways the argument the original author is trying to make here is, someone did this to improve performance by "unrolling" the loop, it is a good example of how you shouldn't do things that a compiler might be able to for you, to try and prioritize making code human readable, since your compiler is smart enough to figure out these micro enhancements.
They are one of the most pernicious error to find because the person reading is doing the exact same mistake as the writer, seeing the first instance is correct, they assume the rest is too.