Applying clang_stringrefize_diagnostics.diff causes DiagnosticIDs.cpp to take >
10 mins to compile with optimizations on. See http://llvm.org/PR9956
-Argyrios
On May 19, 2011, at 12:46 PM, Argyrios Kyrtzidis wrote:
> (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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
