[cfe-commits] [cfe-dev] How to propose patches ?

[Argyrios Kyrtzidis]

(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
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110519/4d22dd3a/attachment.html>