(moved to cfe-commits)
> /// Construct a string ref from a cstring.
> /*implicit*/ StringRef(const char *Str)
> - : Data(Str), Length(::strlen(Str)) {}
> + : Data(Str), Length() {
> + assert(Str && "StringRef cannot be built from a NULL argument");
> + Length = ::strlen(Str); // invoking strlen(NULL) is undefined
> behavior
> + }
>
"Length()" is not necessary.
Could you also add an assert in the StringRef(const char *data, size_t length)
constructor asserting that data is not null or length is 0 ?
> +
> + // Workaround memcmp issue with null pointers (undefined behavior)
> + // by providing a specialized version
> + static int memcmp(const char *Lhs, const char *Rhs, size_t Length) {
> + if (Length == 0) { return 0; }
> + assert(Lhs && "memcmp - Lhs should be non-null when Length is not 0");
> + assert(Rhs && "memcmp - Rhs should be non-null when Length is not 0");
> + return ::memcmp(Lhs,Rhs,Length);
> + }
> +
Is this really necessary ? With the 2 asserts in the constructors we are making
sure that StringRefs point to non-null or their length is zero, and calling
memcmp with zero length is defined, no ?
> Index: tools/libclang/CIndexDiagnostic.cpp
> ===================================================================
> --- tools/libclang/CIndexDiagnostic.cpp (révision 131339)
> +++ tools/libclang/CIndexDiagnostic.cpp (copie de travail)
> @@ -220,7 +220,8 @@
> return createCXString("");
>
> unsigned ID = StoredDiag->Diag.getID();
> - if (const char *Option = DiagnosticIDs::getWarningOptionForDiag(ID)) {
> + llvm::StringRef Option = DiagnosticIDs::getWarningOptionForDiag(ID);
> + if (Option.data()) {
> if (Disable)
Isn't it a bit better and more descriptive if you used !.empty() instead of
.data() ? This applies to the rest of the diffs.
> // -Wsystem-headers is a special case, not driven by the option table.
> It
> // cannot be controlled with -Werror.
> - if (OptEnd-OptStart == 14 && memcmp(OptStart, "system-headers", 14) ==
> 0) {
> + if (Opt == "system-headers") {
> Diags.setSuppressSystemWarnings(!isPositive);
Probably a not-worth-much micro-optimization but with the changes in
lib/Frontend/Warnings.cpp you are introducing multiple strlen calls inside the
loop, maybe use something like llvm::StringRef("system-headers", 14) or
initialize the StringRefs outside the loop ?
Your new diagnostic for non-virtual destructor looks great and useful, I'd
suggest we put it in "most" diagnostic group.
Thanks for your work!
-Argyrios
On May 18, 2011, at 10:07 AM, Matthieu Monrocq wrote:
> Hello clang :)
>
> I have been working recently on a change to the Diagnostics in order to
> enrich the current system, the end goal being to offer additional
> explanations and help to the user (on top of the already displayed error
> message).
>
> I have proposed some patches on cfe-commits some time ago, and then
> re-proposed them (rebased on current ToT), but I haven't yet heard from
> anyone, so I guess I am doing something wrong.
>
> At the moment, I simply send my proposals on cfe-commits (which seemed
> appropriate), but perhaps that I should send them on this list instead ?
>
> - Is there someone that I should put in copy ? (and where to find this
> person, as I suppose it would depend on the area of expertise)
>
> - Would it be better to ask for commit access, and have my changes validated
> after the fact ?
>
> I tried to look about it on the clang website, but it does not seem to be
> mentioned.
>
>
> Here is an excerpt of the last mail I sent, with the appropriate patches in
> attachment:
>
> 1. A simple patch in StringRef.h:
> > put assertions in StringRef(char const*) because it should not be
> > constructed from a null pointer (strlen has undefined behavior)
> > replace memcmp with a version with asserts to against guard against null
> > arguments (because the default constructor of StringRef creates a null
> > pointer)
>
> No functional change expected, merely an easier diagnostic (it sure helped me
> track down a bug I had). I didn't notice any slow-down on my Debug+Assert
> build playing the tests.
>
> This can be found in llvm_stringref_undefined_behavior.diff.
>
>
> 2. A StringRef-ication of the DiagnosticIDs API and internals.
>
> Simple grunt work, no functional change expected. I took the opportunity to
> make some cleanup in lib/Lex/Pragma.cpp and lib/Frontend/Warnings.cpp taking
> advantages of StringRef.
>
> This can be found in clang_stringrefize_diagnostics.diff
>
>
> 3. A simple new diagnostic for non-virtual destructor in polymorphic
> classes.
>
> The difference with the existing diagnostic is that instead of warning at the
> definition of the class, it instead warns when invoking `delete` on it.
> Hopefully reducing the number of false positives.
>
> Unfortunately it is incomplete since a "final" class should not trigger this
> warning but this information does not seem to be available in the AST. I've
> put a FIXME near the code.
>
> It is interestingly a very small patch, which can be found in
> clang_non_virtual_destructor.diff
>
>
> The tests passes for all 3 patches (whether individually or as a group), at
> least as much as tests ever passed on my system (~80 unexpected failures
> because of my msys environment).
>
>
> -- Matthieu
> <llvm_stringref_undefined_behavior.diff><clang_non_virtual_destructor.diff><clang_stringrefize_diagnostics.diff>_______________________________________________
> cfe-dev mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
