[PATCH] Add back 'remark' to libclang interface

[Alp Toker]

On 28/04/2014 10:11, Tobias Grosser wrote:
> Hi,
>
> in commit 207319 the remark libclang interface was removed by Alp due 
> to backward compatibility concerns.
>
> The idea of this patch (and the identical original patch) is to expose 
> 'remark' Diagnostics in the libclang interface. Is it implemented by
> adding a new diagnostics type to the libclang diagnostic enum 
> (obviously not changing any existing enum values). This is the most 
> straightforward implementation.
>
> The concerns raised in the revert commit where:
>
>   It trivially broke almost any stable application checking for
>   Severity >= CXDiagnostic_Error or indeed any other kind of severity
>   comparison upon encountering a 'remark'.
>
> To not misunderstand this comment, if the user does not explicitly 
> enable 'remarks', libclang based applications are _not_ broken and 
> continue to work.
>
> An issue may appear if both the user enables 'remark' diagnostics 
> explicitly and an application assumes that the set of diagnostic
> types does never change and, based on this assumption, contains code 
> that checks the diagnostic type without handling unknown diagnostic 
> types gracefully.
>
> This can be seen as backward compatibility issue, but my personal
> interpretation is that the application is relying here on undocumented
> assumptions. As possible issues only manifest in case the user
> explicitly requests remarks, I believe the proposed patch is fine.
>
> Alp, what is the actual use case that would break? Do you have an 
> application that relies on these assumptions? How likely is it that a 
> user will enable remark diagnostics for this application?

Hi Tobias,

I'll re-send this here for the benefit of the thread.

This is the typical usage of this interface from 
tools/c-index-test/c-index-test.c:

static int checkForErrors(CXTranslationUnit TU) {
...
     if (clang_getDiagnosticSeverity(Diag) >= CXDiagnostic_Error) {
       DiagStr = clang_formatDiagnostic(Diag,
clang_defaultDiagnosticDisplayOptions());
       fprintf(stderr, "%s\n", clang_getCString(DiagStr));
       clang_disposeString(DiagStr);
       clang_disposeDiagnostic(Diag);
       return -1;
     }
...
}

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.

It's therefore not viable to extend the current API as proposed.

I suggest continuing to map these to warning as in my revert and adding 
an accessor when there's a strong need, or extending the enum when the 
libclang major version is bumped -- whichever happens sooner.

Alp.


>
> Any comments?
> Tobias
>
> P.S: Alp apparently has a better solution. I let him explain this 
> solution himself.

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