[PATCH] Add back 'remark' to libclang interface

[Alp Toker]

On 28/04/2014 12:59, Tobias Grosser wrote:
> On 28/04/2014 13:41, Tobias Grosser wrote:
>> On 28/04/2014 13:08, Alp Toker wrote:
>>>
>>> On 28/04/2014 11:11, Tobias Grosser wrote:
>>>>
>>>>
>>>> On 28/04/2014 11:16, Alp Toker wrote:
>>>>> CXDiagnostic with value 5 is higher than CXDiagnostic_Error and there
>>>>> are many applications using the outlined pattern that break following
>>>>> the change, either by crashing or mis-categorising diagnostics as 
>>>>> fatal
>>>>> errors.
>>>>
>>>> I see that the above categorizes the diagnostic as an error. This is
>>>> obviously incorrect. Nevertheless, it should just cause an error
>>>> message. I do not see how/why this would crash.
>>>
>>> It hits an unreachable condition due to an unhandled case and crashes.
>>
>> I consider this an application bug.
>>
>> My quick look on ohloh did not find such usage. (Most results are
>> duplicates of clang code, but e.g. ycm does the right thing and just
>> maps to 'E' in case of unknown diagnostics). Obviously, there may still
>> be other applications.
>
> I looked a little further. There are only seven projects on ohloh 
> which actually look at the diagnostic level (all others just have a 
> copy of clang somewhere). All seem to do something sensible (either 
> map to an unknown diagnostic string, map it as an error or to a very 
> high log level).
>
> So the assumptions we are afraid of are to the very least not very 
> widespread on ohloh.
>
>

Tobias, handling remarks as errors is not OK. It causes applications 
that used to accept valid input to now bail out. That's a break that can 
be triggered by users. At least a couple of these apps also seem to 
enter an undefined state.

I've emphasised your examples and added some more lines from the context 
to point out the real impact:



> ## YouCompleteMe
>
> char DiagnosticSeverityToType( CXDiagnosticSeverity severity ) {
>   switch ( severity ) {
>     case CXDiagnostic_Ignored:
>     case CXDiagnostic_Note:
>       return 'I';
>
>     case CXDiagnostic_Warning:
>       return 'W';
>
>     case CXDiagnostic_Error:
>     case CXDiagnostic_Fatal:
>       return 'E';

Handled as an error:

>
> *    default: **
> **      return 'E'; *
>   }
>
> ## go-clang
>
> func (ds DiagnosticSeverity) String() string {
>     switch ds {
>     case Diagnostic_Ignored:
>         return "Ignored"
>     case Diagnostic_Note:
>         return "Note"
>     case Diagnostic_Warning:
>         return "Warning"
>     case Diagnostic_Error:
>         return "Error"
>     case Diagnostic_Fatal:
>         return "Fatal"
>     default:
>         return "Invalid"
>     }
> }

Handled as an error:

>
> ## rust-bindgen
>
>                 if d.severity() >= CXDiagnostic_Error {
>                     c_err = true;
>                 }
>
> ## bindgen.clay
>
>          if (clang_getDiagnosticSeverity(diag) >= CXDiagnostic_Error)
>             error? = true;
>

Treated as an error:

> ## oovcde
>
> *          if(sev >= CXDiagnostic_Error) **
> **              errType = ET_CompileErrors; *
>           else
>               errType = ET_CompileWarnings;
>
>           }
>
> ## rtags
>
>         int logLevel = INT_MAX;
>         const CXDiagnosticSeverity severity = 
> clang_getDiagnosticSeverity(diagnostic);
>         switch (severity) {
>         case CXDiagnostic_Fatal:
>         case CXDiagnostic_Error:
>             logLevel = Error;
>             break;
>         case CXDiagnostic_Warning:
>             logLevel = Warning;
>             break;
>         case CXDiagnostic_Note:
>             logLevel = Debug;
>             break;
>         case CXDiagnostic_Ignored:
>             break;
>         }
You omitted the next few lines which proceed with logLevel = INT_MAX and 
treat the remarks as errors:

        }

         const CXSourceLocation diagLoc = 
clang_getDiagnosticLocation(diagnostic);
         const Location loc = createLocation(diagLoc, 0);
         const uint32_t fileId = loc.fileId();
         if (mVisitedFiles.contains(fileId)) {
*            if (severity >= CXDiagnostic_Error)**
**                ++mData->errors[fileId];*


Mapping remarks to warnings for now as I've done at least permits these 
applications to function as the developer intended, which is the highest 
priority for libclang users.

Each traditional diagnostic severity has its own semantics:

*Note*: Attached to another diagnostic.
*Warning*: A message to the user.
*Error*: A message about an issue that prevents compilation.
*FatalError*: A message about an issue that has broken the state of the 
frontend or diagnostic engine, such that serialization may not be reliable.

Your Remark severity has the same semantics as Warning, only with a 
different display string.

You still haven't answered why you chose to add a new diagnostic level 
to clang, along with all the associated code churn. Given that the 
semantics are identical this could have been trivially implemented as a 
one-bit modifier on Warning, especially since the only two uses of 
Remark<"%0"> are pass-throughs.

I don't understand why you're pushing for something that's flawed when 
there's potentially an elegant solution in sight and some serious issues 
with the proposed one.

Alp.


-- 
http://www.nuanti.com
the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140428/6a2e60d5/attachment.html>