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