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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
