[PATCH] Add back 'remark' to libclang interface

[Tobias Grosser]

On 28/04/2014 11:16, Alp Toker wrote:
>
> 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.

Thanks Alp.

> 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.

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.

As remarks are _only_ emitted on user request and are promoted as tool 
to understand the compilation, I would claim it is acceptable that
older tools not supporting remarks abort with an error. The user should 
always be able to remove his '-Rpass=inline' option which the tools does
not properly support anyway. In fact, aborting with an error seems 
clearer than just emitting more

Do you have an application / use case, where such an error would be 
unacceptable or fully misleading?

> 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.

This is definitely incomplete and I would like to avoid it. If we really 
conclude that this is necessary, we could probably make this optional 
(on by default) and allow newer libraries to retrieve the correct 
remarks. Is adding this complexity necessary?

Cheers,
Tobias