- User Since
- Mar 14 2013, 3:16 PM (210 w, 5 d)
I'm not certain of a good way to test it, but I have a question about the value you picked for init_priority. My understanding of the values starting from 101 is that 1-100 are reserved for implementation use. Is that understanding correct? If so, you may want to pick a value below 100 to ensure there's not an arms race with the user. I believe this may require some alteration to SemaDeclAttr.cpp to not diagnose when this is coming from a system header. Otherwise, what's to stop the user from having something marked constructor(101) that attempts to use a stream, but can't because they're not initialized yet?
Mon, Mar 27
Because I expect this to be fairly uncontroversial, I'll give it a week in case folks have comments, then commit (and will handle anything in post-commit review).
Sun, Mar 26
Sat, Mar 25
Thu, Mar 23
Wed, Mar 22
Tue, Mar 21
This change was reverted in r298470. The use of the <assert.h> include is a problem because this is not a clang-supplied header file. Also, there's a (good) question about what array decay is happening in the assert(false) test case.
I've commit in r298421.
Missed a case for UnaryExprOrTypeTraitExpr.
Mon, Mar 20
Sun, Mar 19
Commit in r298229. If we want to reword the hicpp-no-assembler diagnostic, we can do so in a follow-up patch.
I'd also like to see some tests in Misc that confirm the attribute is being attached to the appropriate AST nodes for declarations, statements, and at namespace scope.
Sat, Mar 18
I really like the way this feature is shaping up! I apologize for how long it took to review the code (I was out for work for two weeks and this is a pretty extensive patch). Thank you for the continued efforts on this.
Aside from the request for a FIXME and a decision from the author on error vs warning, the code LGTM. Feel free to make a decision and commit without further review.
Thu, Mar 16
Wed, Mar 15
I've commit in r297882, thanks!
LGTM! I'll go ahead and commit for you.
Tue, Mar 14
Mon, Mar 13
I'm curious what kind of results you get when running this over a large code base. There are definitely times when using == or != for a specific value is *way* cleaner than using a switch statement, and I worry about this being so chatty that it needs to be disabled by default.
I've commit in r297671, thanks!
Can someone more familiar with UNIX permissions review the new changes before we land them?
LGTM as well; I've commit in r297617, thank you!
Sun, Mar 12
Committed in r297592, thank you!
Wed, Mar 8
My biggest concern with this is that it doesn't really set the file permissions on Windows, it just sets the file *attributes*. It's perfectly cromulent for a file to have the read-only attribute set but still not have read permissions for the current user, or to not have the read-only attribute set and still not be writable for the current user, for instance. Is that something we should be caring about?
This check is also somewhat similar to DCL57-CPP from the CERT C++ secure coding guidelines (https://www.securecoding.cert.org/confluence/display/cplusplus/DCL57-CPP.+Do+not+let+exceptions+escape+from+destructors+or+deallocation+functions). If you're interested in extending this check to cover deallocation functions, which have the same concerns as destructors, then it could also be added to the CERT module as well.
Sat, Mar 4
Missing documentation changes for the new options.
LGTM, thank you!
Fri, Mar 3
This change is missing test cases.
Thu, Mar 2
Wed, Mar 1
Out of curiosity, have you run this over libc++ or libstdc++ test suites involving std::random_shuffle? If so, were the results acceptable?