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