<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Applying clang_stringrefize_diagnostics.diff causes DiagnosticIDs.cpp to take > 10 mins to compile with optimizations on. See <a href="http://llvm.org/PR9956">http://llvm.org/PR9956</a></div><div><br></div><div>-Argyrios</div><div><br></div><div><div><div>On May 19, 2011, at 12:46 PM, Argyrios Kyrtzidis wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>(moved to cfe-commits)</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span"> /// Construct a string ref from a cstring.</font></div><div><font class="Apple-style-span"> /*implicit*/ StringRef(const char *Str)</font></div><div><font class="Apple-style-span">- : Data(Str), Length(::strlen(Str)) {}</font></div><div><font class="Apple-style-span">+ : Data(Str), Length() {</font></div><div><font class="Apple-style-span">+ assert(Str && "StringRef cannot be built from a NULL argument");</font></div><div><font class="Apple-style-span">+ Length = ::strlen(Str); // invoking strlen(NULL) is undefined behavior</font></div><div><font class="Apple-style-span">+ }</font></div><div><font class="Apple-style-span"> </font></div></blockquote></div><div><div><br></div></div><div>"Length()" is not necessary.</div><div>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 ?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span">+ </font></div><div><font class="Apple-style-span">+ // Workaround memcmp issue with null pointers (undefined behavior)</font></div><div><font class="Apple-style-span">+ // by providing a specialized version</font></div><div><font class="Apple-style-span">+ static int memcmp(const char *Lhs, const char *Rhs, size_t Length) {</font></div><div><font class="Apple-style-span">+ if (Length == 0) { return 0; }</font></div><div><font class="Apple-style-span">+ assert(Lhs && "memcmp - Lhs should be non-null when Length is not 0");</font></div><div><font class="Apple-style-span">+ assert(Rhs && "memcmp - Rhs should be non-null when Length is not 0");</font></div><div><font class="Apple-style-span">+ return ::memcmp(Lhs,Rhs,Length);</font></div><div><font class="Apple-style-span">+ }</font></div><div><font class="Apple-style-span">+ </font></div></blockquote></div><div><div><br></div></div><div>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 ?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span">Index: tools/libclang/CIndexDiagnostic.cpp</font></div><div><font class="Apple-style-span">===================================================================</font></div><div><font class="Apple-style-span">--- tools/libclang/CIndexDiagnostic.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span"> </font></span><font class="Apple-style-span">(révision 131339)</font></div><div><font class="Apple-style-span">+++ tools/libclang/CIndexDiagnostic.cpp</font><span class="Apple-tab-span" style="white-space:pre"><font class="Apple-style-span"> </font></span><font class="Apple-style-span">(copie de travail)</font></div><div><font class="Apple-style-span">@@ -220,7 +220,8 @@</font></div><div><font class="Apple-style-span"> return createCXString("");</font></div><div><font class="Apple-style-span"> </font></div><div><font class="Apple-style-span"> unsigned ID = StoredDiag->Diag.getID();</font></div><div><font class="Apple-style-span">- if (const char *Option = DiagnosticIDs::getWarningOptionForDiag(ID)) {</font></div><div><font class="Apple-style-span">+ llvm::StringRef Option = DiagnosticIDs::getWarningOptionForDiag(ID);</font></div><div><font class="Apple-style-span">+ if (Option.data()) {</font></div><div><font class="Apple-style-span"> if (Disable)</font></div></blockquote></div><div><div><br></div></div><div>Isn't it a bit better and more descriptive if you used !.empty() instead of .data() ? This applies to the rest of the diffs.</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span"> // -Wsystem-headers is a special case, not driven by the option table. It</font></div><div><font class="Apple-style-span"> // cannot be controlled with -Werror.</font></div><div><font class="Apple-style-span">- if (OptEnd-OptStart == 14 && memcmp(OptStart, "system-headers", 14) == 0) {</font></div><div><font class="Apple-style-span">+ if (Opt == "system-headers") {</font></div><div><font class="Apple-style-span"> Diags.setSuppressSystemWarnings(!isPositive);</font></div></blockquote></div><div><div><br></div></div><div>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 ?</div><div><br></div><div><br></div><div>Your new diagnostic for non-virtual destructor looks great and useful, I'd suggest we put it in "most" diagnostic group.</div><div><br></div><div>Thanks for your work!</div><div><br></div><div>-Argyrios</div><br><div><div>On May 18, 2011, at 10:07 AM, Matthieu Monrocq wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hello clang :)<br><br>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).<br>
<br>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.<br><br>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 ?<br>
<br>- 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)<br><br>- Would it be better to ask for commit access, and have my changes validated after the fact ?<br>
<br>I tried to look about it on the clang website, but it does not seem to be mentioned.<br><br><br>Here is an excerpt of the last mail I sent, with the appropriate patches in attachment:<br><br>1. A simple patch in StringRef.h:<br>
> put assertions in StringRef(char const*) because it should not be constructed from a null pointer (strlen has undefined behavior)<br>
> replace memcmp with a version with asserts to against guard against null arguments (because the default constructor of StringRef creates a null pointer)<br>
<br>
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.<br>
<br>
This can be found in llvm_stringref_undefined_behavior.diff.<br><div id=":9z">
<br>
<br>
2. A StringRef-ication of the DiagnosticIDs API and internals.<br>
<br>
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.<br>
<br>
This can be found in clang_stringrefize_diagnostics.diff<br>
<br>
<br>
3. A simple new diagnostic for non-virtual destructor in polymorphic<br>
classes.<br>
<br>
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.<br>
<br>
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.<br>
<br>
It is interestingly a very small patch, which can be found in clang_non_virtual_destructor.diff<br>
<br>
<br>
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).</div><br><br>-- Matthieu<br>
<span><llvm_stringref_undefined_behavior.diff></span><span><clang_non_virtual_destructor.diff></span><span><clang_stringrefize_diagnostics.diff></span>_______________________________________________<br>cfe-dev mailing list<br><a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br></blockquote></div><br></div>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote></div><br></div></body></html>