Remove extraInfo parameter from compiler diagnostic functions
This makes error messages more consistent. It was not clear what was
supposed to go to the extraInfo parameter, and previously it was
mostly being misused, resulting in poorly formatted error messages.
Sometimes the order of parameters to the diagnostic functions like
error() and warning() was wrong altogether. The diagnostics API is
simpler when there's only the "reason" and "token" parameters that
have clear meaning and that are separated by consistent punctuation
in the output.
Fixes error messages like
"redifinition interface block member"
to be grammatically reasonable like the rest of the error messages. For
other error messages, punctuation is added to make them clearer. Example:
"invalid layout qualifier location requires an argument"
is changed to
"invalid layout qualifier: location requires an argument".
Extra spaces are also removed from the beginning of error messages.
BUG=angleproject:1670
BUG=angleproject:911
TEST=angle_unittests
Change-Id: Id5fb1a1f2892fad2b796aaef47ffb07e9d79759c
Reviewed-on: https://chromium-review.googlesource.com/420789
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Olli Etuaho <oetuaho@nvidia.com>
diff --git a/src/compiler/translator/ParseContext.cpp b/src/compiler/translator/ParseContext.cpp
index 30e8b9d..4eab295 100644
--- a/src/compiler/translator/ParseContext.cpp
+++ b/src/compiler/translator/ParseContext.cpp
@@ -225,35 +225,28 @@
//
// Used by flex/bison to output all syntax and parsing errors.
//
-void TParseContext::error(const TSourceLoc &loc,
- const char *reason,
- const char *token,
- const char *extraInfo)
+void TParseContext::error(const TSourceLoc &loc, const char *reason, const char *token)
{
- mDiagnostics.error(loc, reason, token, extraInfo);
+ mDiagnostics.error(loc, reason, token);
}
-void TParseContext::warning(const TSourceLoc &loc,
- const char *reason,
- const char *token,
- const char *extraInfo)
+void TParseContext::warning(const TSourceLoc &loc, const char *reason, const char *token)
{
- mDiagnostics.warning(loc, reason, token, extraInfo);
+ mDiagnostics.warning(loc, reason, token);
}
void TParseContext::outOfRangeError(bool isError,
const TSourceLoc &loc,
const char *reason,
- const char *token,
- const char *extraInfo)
+ const char *token)
{
if (isError)
{
- error(loc, reason, token, extraInfo);
+ error(loc, reason, token);
}
else
{
- warning(loc, reason, token, extraInfo);
+ warning(loc, reason, token);
}
}
@@ -262,10 +255,10 @@
//
void TParseContext::assignError(const TSourceLoc &line, const char *op, TString left, TString right)
{
- std::stringstream extraInfoStream;
- extraInfoStream << "cannot convert from '" << right << "' to '" << left << "'";
- std::string extraInfo = extraInfoStream.str();
- error(line, "", op, extraInfo.c_str());
+ std::stringstream reasonStream;
+ reasonStream << "cannot convert from '" << right << "' to '" << left << "'";
+ std::string reason = reasonStream.str();
+ error(line, reason.c_str(), op);
}
//
@@ -273,11 +266,12 @@
//
void TParseContext::unaryOpError(const TSourceLoc &line, const char *op, TString operand)
{
- std::stringstream extraInfoStream;
- extraInfoStream << "no operation '" << op << "' exists that takes an operand of type "
- << operand << " (or there is no acceptable conversion)";
- std::string extraInfo = extraInfoStream.str();
- error(line, " wrong operand type", op, extraInfo.c_str());
+ std::stringstream reasonStream;
+ reasonStream << "wrong operand type - no operation '" << op
+ << "' exists that takes an operand of type " << operand
+ << " (or there is no acceptable conversion)";
+ std::string reason = reasonStream.str();
+ error(line, reason.c_str(), op);
}
//
@@ -288,12 +282,13 @@
TString left,
TString right)
{
- std::stringstream extraInfoStream;
- extraInfoStream << "no operation '" << op << "' exists that takes a left-hand operand of type '"
- << left << "' and a right operand of type '" << right
- << "' (or there is no acceptable conversion)";
- std::string extraInfo = extraInfoStream.str();
- error(line, " wrong operand types ", op, extraInfo.c_str());
+ std::stringstream reasonStream;
+ reasonStream << "wrong operand types - no operation '" << op
+ << "' exists that takes a left-hand operand of type '" << left
+ << "' and a right operand of type '" << right
+ << "' (or there is no acceptable conversion)";
+ std::string reason = reasonStream.str();
+ error(line, reason.c_str(), op);
}
void TParseContext::checkPrecisionSpecified(const TSourceLoc &line,
@@ -370,10 +365,6 @@
return false;
}
- const char *symbol = 0;
- if (symNode != 0)
- symbol = symNode->getSymbol().c_str();
-
const char *message = 0;
switch (node->getQualifier())
{
@@ -448,7 +439,7 @@
if (message == 0 && binaryNode == 0 && symNode == 0)
{
- error(line, " l-value required", op);
+ error(line, "l-value required", op);
return false;
}
@@ -464,17 +455,18 @@
//
if (symNode)
{
- std::stringstream extraInfoStream;
- extraInfoStream << "\"" << symbol << "\" (" << message << ")";
- std::string extraInfo = extraInfoStream.str();
- error(line, " l-value required", op, extraInfo.c_str());
+ const char *symbol = symNode->getSymbol().c_str();
+ std::stringstream reasonStream;
+ reasonStream << "l-value required (" << message << " \"" << symbol << "\")";
+ std::string reason = reasonStream.str();
+ error(line, reason.c_str(), op);
}
else
{
- std::stringstream extraInfoStream;
- extraInfoStream << "(" << message << ")";
- std::string extraInfo = extraInfoStream.str();
- error(line, " l-value required", op, extraInfo.c_str());
+ std::stringstream reasonStream;
+ reasonStream << "l-value required (" << message << ")";
+ std::string reason = reasonStream.str();
+ error(line, reason.c_str(), op);
}
return false;
@@ -697,7 +689,7 @@
ASSERT(!argType.isArray());
if (!argType.sameElementType(type))
{
- error(line, "Array constructor argument has an incorrect type", "Error");
+ error(line, "Array constructor argument has an incorrect type", "constructor");
return false;
}
}
@@ -712,7 +704,7 @@
if (i >= args->size() || (*args)[i]->getAsTyped()->getType() != *fields[i]->type())
{
error(line, "Structure constructor arguments do not match structure fields",
- "Error");
+ "constructor");
return false;
}
}
@@ -767,7 +759,10 @@
{
if (ContainsSampler(*pType.userDef))
{
- error(line, reason, getBasicString(pType.type), "(structure contains a sampler)");
+ std::stringstream reasonStream;
+ reasonStream << reason << " (structure contains a sampler)";
+ std::string reasonStr = reasonStream.str();
+ error(line, reasonStr.c_str(), getBasicString(pType.type));
return false;
}
@@ -790,7 +785,10 @@
{
if (ContainsImage(*pType.userDef))
{
- error(line, reason, getBasicString(pType.type), "(structure contains an image)");
+ std::stringstream reasonStream;
+ reasonStream << reason << " (structure contains an image)";
+ std::string reasonStr = reasonStream.str();
+ error(line, reasonStr.c_str(), getBasicString(pType.type));
return false;
}
@@ -822,8 +820,8 @@
{
if (layoutQualifier.location != -1)
{
- error(location, "invalid layout qualifier:", "location",
- "only valid on program inputs and outputs");
+ error(location, "invalid layout qualifier: only valid on program inputs and outputs",
+ "location");
}
}
@@ -1081,18 +1079,18 @@
TExtensionBehavior::const_iterator iter = extBehavior.find(extension.c_str());
if (iter == extBehavior.end())
{
- error(line, "extension", extension.c_str(), "is not supported");
+ error(line, "extension is not supported", extension.c_str());
return false;
}
// In GLSL ES, an extension's default behavior is "disable".
if (iter->second == EBhDisable || iter->second == EBhUndefined)
{
- error(line, "extension", extension.c_str(), "is disabled");
+ error(line, "extension is disabled", extension.c_str());
return false;
}
if (iter->second == EBhWarn)
{
- warning(line, "extension", extension.c_str(), "is being used");
+ warning(line, "extension is being used", extension.c_str());
return true;
}
@@ -1156,17 +1154,15 @@
if (layoutQualifier.matrixPacking != EmpUnspecified)
{
- error(identifierLocation, "layout qualifier",
- getMatrixPackingString(layoutQualifier.matrixPacking),
- "only valid for interface blocks");
+ error(identifierLocation, "layout qualifier only valid for interface blocks",
+ getMatrixPackingString(layoutQualifier.matrixPacking));
return;
}
if (layoutQualifier.blockStorage != EbsUnspecified)
{
- error(identifierLocation, "layout qualifier",
- getBlockStorageString(layoutQualifier.blockStorage),
- "only valid for interface blocks");
+ error(identifierLocation, "layout qualifier only valid for interface blocks",
+ getBlockStorageString(layoutQualifier.blockStorage));
return;
}
@@ -1266,7 +1262,7 @@
if (mShaderVersion < versionRequired)
{
- error(location, "invalid layout qualifier:", layoutQualifierName.c_str(), "not supported");
+ error(location, "invalid layout qualifier: not supported", layoutQualifierName.c_str());
}
}
@@ -1278,8 +1274,10 @@
{
if (localSize[i] != -1)
{
- error(location, "invalid layout qualifier:", getWorkGroupSizeString(i),
- "only valid when used with 'in' in a compute shader global layout declaration");
+ error(location,
+ "invalid layout qualifier: only valid when used with 'in' in a compute shader "
+ "global layout declaration",
+ getWorkGroupSizeString(i));
return false;
}
}
@@ -1292,8 +1290,8 @@
{
if (internalFormat != EiifUnspecified)
{
- error(location, "invalid layout qualifier:", getImageInternalFormatString(internalFormat),
- "only valid when used with images");
+ error(location, "invalid layout qualifier: only valid when used with images",
+ getImageInternalFormatString(internalFormat));
return false;
}
return true;
@@ -1310,8 +1308,11 @@
TIntermTyped *argument = (*(fnCall->getSequence()))[i]->getAsTyped();
if (!checkCanBeLValue(argument->getLine(), "assign", argument))
{
+ TString unmangledName =
+ TFunction::unmangleName(fnCall->getFunctionSymbolInfo()->getName());
error(argument->getLine(),
- "Constant value cannot be passed for 'out' or 'inout' parameters.", "Error");
+ "Constant value cannot be passed for 'out' or 'inout' parameters.",
+ unmangledName.c_str());
return;
}
}
@@ -1617,10 +1618,11 @@
{
if (qualifier != initializer->getType().getQualifier())
{
- std::stringstream extraInfoStream;
- extraInfoStream << "'" << variable->getType().getCompleteString() << "'";
- std::string extraInfo = extraInfoStream.str();
- error(line, " assigning non-constant to", "=", extraInfo.c_str());
+ std::stringstream reasonStream;
+ reasonStream << "assigning non-constant to '" << variable->getType().getCompleteString()
+ << "'";
+ std::string reason = reasonStream.str();
+ error(line, reason.c_str(), "=");
variable->getType().setQualifier(EvqTemporary);
return true;
}
@@ -2300,13 +2302,12 @@
if (mComputeShaderLocalSize[i] < 1 ||
mComputeShaderLocalSize[i] > maxComputeWorkGroupSizeValue)
{
- std::stringstream errorMessageStream;
- errorMessageStream << "Value must be at least 1 and no greater than "
- << maxComputeWorkGroupSizeValue;
- const std::string &errorMessage = errorMessageStream.str();
+ std::stringstream reasonStream;
+ reasonStream << "invalid value: Value must be at least 1 and no greater than "
+ << maxComputeWorkGroupSizeValue;
+ const std::string &reason = reasonStream.str();
- error(typeQualifier.line, "invalid value:", getWorkGroupSizeString(i),
- errorMessage.c_str());
+ error(typeQualifier.line, reason.c_str(), getWorkGroupSizeString(i));
return;
}
}
@@ -2324,8 +2325,8 @@
if (typeQualifier.qualifier != EvqUniform)
{
- error(typeQualifier.line, "invalid qualifier:",
- getQualifierString(typeQualifier.qualifier), "global layout must be uniform");
+ error(typeQualifier.line, "invalid qualifier: global layout must be uniform",
+ getQualifierString(typeQualifier.qualifier));
return;
}
@@ -2412,7 +2413,7 @@
// Check that non-void functions have at least one return statement.
if (mCurrentFunctionType->getBasicType() != EbtVoid && !mFunctionReturnsValue)
{
- error(location, "function does not return a value:", "", function.getName().c_str());
+ error(location, "function does not return a value:", function.getName().c_str());
}
if (functionBody == nullptr)
@@ -2479,8 +2480,8 @@
}
if ((*function)->getReturnType().getBasicType() != EbtVoid)
{
- error(location, "", (*function)->getReturnType().getBasicString(),
- "main function cannot return a value");
+ error(location, "main function cannot return a value",
+ (*function)->getReturnType().getBasicString());
}
}
@@ -2584,7 +2585,7 @@
{
if (!prevSym->isFunction())
{
- error(location, "redefinition", function->getName().c_str(), "function");
+ error(location, "redefinition of a function", function->getName().c_str());
}
}
else
@@ -2757,8 +2758,8 @@
if (typeQualifier.qualifier != EvqUniform)
{
- error(typeQualifier.line, "invalid qualifier:", getQualifierString(typeQualifier.qualifier),
- "interface blocks must be uniform");
+ error(typeQualifier.line, "invalid qualifier: interface blocks must be uniform",
+ getQualifierString(typeQualifier.qualifier));
}
if (typeQualifier.invariant)
@@ -2788,7 +2789,7 @@
TSymbol *blockNameSymbol = new TInterfaceBlockName(&blockName);
if (!symbolTable.declare(blockNameSymbol))
{
- error(nameLine, "redefinition", blockName.c_str(), "interface block name");
+ error(nameLine, "redefinition of an interface block name", blockName.c_str());
}
// check for sampler types and apply layout qualifiers
@@ -2798,14 +2799,16 @@
TType *fieldType = field->type();
if (IsSampler(fieldType->getBasicType()))
{
- error(field->line(), "unsupported type", fieldType->getBasicString(),
- "sampler types are not allowed in interface blocks");
+ error(field->line(),
+ "unsupported type - sampler types are not allowed in interface blocks",
+ fieldType->getBasicString());
}
if (IsImage(fieldType->getBasicType()))
{
- error(field->line(), "unsupported type", fieldType->getBasicString(),
- "image types are not allowed in interface blocks");
+ error(field->line(),
+ "unsupported type - image types are not allowed in interface blocks",
+ fieldType->getBasicString());
}
const TQualifier qualifier = fieldType->getQualifier();
@@ -2831,8 +2834,8 @@
if (fieldLayoutQualifier.blockStorage != EbsUnspecified)
{
- error(field->line(), "invalid layout qualifier:",
- getBlockStorageString(fieldLayoutQualifier.blockStorage), "cannot be used here");
+ error(field->line(), "invalid layout qualifier: cannot be used here",
+ getBlockStorageString(fieldLayoutQualifier.blockStorage));
}
if (fieldLayoutQualifier.matrixPacking == EmpUnspecified)
@@ -2841,9 +2844,9 @@
}
else if (!fieldType->isMatrix() && fieldType->getBasicType() != EbtStruct)
{
- warning(field->line(), "extraneous layout qualifier:",
- getMatrixPackingString(fieldLayoutQualifier.matrixPacking),
- "only has an effect on matrix types");
+ warning(field->line(),
+ "extraneous layout qualifier: only has an effect on matrix types",
+ getMatrixPackingString(fieldLayoutQualifier.matrixPacking));
}
fieldType->setLayoutQualifier(fieldLayoutQualifier);
@@ -2880,8 +2883,8 @@
if (!symbolTable.declare(fieldVariable))
{
- error(field->line(), "redefinition", field->name().c_str(),
- "interface block member name");
+ error(field->line(), "redefinition of an interface block member name",
+ field->name().c_str());
}
}
}
@@ -2895,8 +2898,8 @@
if (!symbolTable.declare(instanceTypeDef))
{
- error(instanceLine, "redefinition", instanceName->c_str(),
- "interface block instance name");
+ error(instanceLine, "redefinition of an interface block instance name",
+ instanceName->c_str());
}
symbolId = instanceTypeDef->getUniqueId();
@@ -2918,11 +2921,10 @@
++mStructNestingLevel;
// Embedded structure definitions are not supported per GLSL ES spec.
- // They aren't allowed in GLSL either, but we need to detect this here
- // so we don't rely on the GLSL compiler to catch it.
+ // ESSL 1.00.17 section 10.9. ESSL 3.00.6 section 12.11.
if (mStructNestingLevel > 1)
{
- error(line, "", "Embedded struct definitions are not allowed");
+ error(line, "Embedded struct definitions are not allowed", "struct");
}
}
@@ -2951,7 +2953,7 @@
reasonStream << "Reference of struct type " << field.type()->getStruct()->name().c_str()
<< " exceeds maximum allowed nesting level of " << kWebGLMaxStructNesting;
std::string reason = reasonStream.str();
- error(line, reason.c_str(), field.name().c_str(), "");
+ error(line, reason.c_str(), field.name().c_str());
return;
}
}
@@ -2991,18 +2993,18 @@
{
if (baseExpression->isInterfaceBlock())
{
- error(
- location, "", "[",
- "array indexes for interface blocks arrays must be constant integral expressions");
+ error(location,
+ "array indexes for interface blocks arrays must be constant integral expressions",
+ "[");
}
else if (baseExpression->getQualifier() == EvqFragmentOut)
{
- error(location, "", "[",
- "array indexes for fragment outputs must be constant integral expressions");
+ error(location,
+ "array indexes for fragment outputs must be constant integral expressions", "[");
}
else if (mShaderSpec == SH_WEBGL2_SPEC && baseExpression->getQualifier() == EvqFragData)
{
- error(location, "", "[", "array index for gl_FragData must be constant zero");
+ error(location, "array index for gl_FragData must be constant zero", "[");
}
}
@@ -3027,16 +3029,16 @@
// Error has been already generated if index is not const.
if (indexExpression->getQualifier() == EvqConst)
{
- error(location, "", "[",
- "array index for gl_FragData must be constant zero");
+ error(location, "array index for gl_FragData must be constant zero", "[");
}
safeIndex = 0;
}
else if (!isExtensionEnabled("GL_EXT_draw_buffers"))
{
- outOfRangeError(outOfRangeIndexIsError, location, "", "[",
+ outOfRangeError(outOfRangeIndexIsError, location,
"array index for gl_FragData must be zero when "
- "GL_EXT_draw_buffers is disabled");
+ "GL_EXT_draw_buffers is disabled",
+ "[");
safeIndex = 0;
}
}
@@ -3045,20 +3047,20 @@
{
safeIndex = checkIndexOutOfRange(outOfRangeIndexIsError, location, index,
baseExpression->getArraySize(),
- "array index out of range", "[]");
+ "array index out of range");
}
}
else if (baseExpression->isMatrix())
{
safeIndex = checkIndexOutOfRange(outOfRangeIndexIsError, location, index,
baseExpression->getType().getCols(),
- "matrix field selection out of range", "[]");
+ "matrix field selection out of range");
}
else if (baseExpression->isVector())
{
safeIndex = checkIndexOutOfRange(outOfRangeIndexIsError, location, index,
baseExpression->getType().getNominalSize(),
- "vector field selection out of range", "[]");
+ "vector field selection out of range");
}
ASSERT(safeIndex >= 0);
@@ -3086,15 +3088,14 @@
const TSourceLoc &location,
int index,
int arraySize,
- const char *reason,
- const char *token)
+ const char *reason)
{
if (index >= arraySize || index < 0)
{
- std::stringstream extraInfoStream;
- extraInfoStream << "'" << index << "'";
- std::string extraInfo = extraInfoStream.str();
- outOfRangeError(outOfRangeIndexIsError, location, reason, token, extraInfo.c_str());
+ std::stringstream reasonStream;
+ reasonStream << reason << " '" << index << "'";
+ std::string token = reasonStream.str();
+ outOfRangeError(outOfRangeIndexIsError, location, reason, "[]");
if (index < 0)
{
return 0;
@@ -3251,8 +3252,8 @@
}
else if (qualifierType == "location")
{
- error(qualifierTypeLine, "invalid layout qualifier", qualifierType.c_str(),
- "location requires an argument");
+ error(qualifierTypeLine, "invalid layout qualifier: location requires an argument",
+ qualifierType.c_str());
}
else if (qualifierType == "rgba32f")
{
@@ -3339,8 +3340,10 @@
checkLayoutQualifierSupported(qualifierTypeLine, qualifierType, 310);
if (intValue < 1)
{
- std::string errorMessage = std::string(getWorkGroupSizeString(index)) + " must be positive";
- error(intValueLine, "out of range:", intValueString.c_str(), errorMessage.c_str());
+ std::stringstream reasonStream;
+ reasonStream << "out of range: " << getWorkGroupSizeString(index) << " must be positive";
+ std::string reason = reasonStream.str();
+ error(intValueLine, reason.c_str(), intValueString.c_str());
}
(*localSize)[index] = intValue;
}
@@ -3359,8 +3362,8 @@
// must check that location is non-negative
if (intValue < 0)
{
- error(intValueLine, "out of range:", intValueString.c_str(),
- "location must be non-negative");
+ error(intValueLine, "out of range: location must be non-negative",
+ intValueString.c_str());
}
else
{
@@ -3406,6 +3409,24 @@
&mDiagnostics);
}
+TFieldList *TParseContext::combineStructFieldLists(TFieldList *processedFields,
+ const TFieldList *newlyAddedFields,
+ const TSourceLoc &location)
+{
+ for (TField *field : *newlyAddedFields)
+ {
+ for (TField *oldField : *processedFields)
+ {
+ if (oldField->name() == field->name())
+ {
+ error(location, "duplicate field name in structure", field->name().c_str());
+ }
+ }
+ processedFields->push_back(field);
+ }
+ return processedFields;
+}
+
TFieldList *TParseContext::addStructDeclaratorListWithQualifiers(
const TTypeQualifierBuilder &typeQualifierBuilder,
TPublicType *typeSpecifier,
@@ -3485,7 +3506,7 @@
TVariable *userTypeDef = new TVariable(structName, *structureType, true);
if (!symbolTable.declare(userTypeDef))
{
- error(nameLine, "redefinition", structName->c_str(), "struct");
+ error(nameLine, "redefinition of a struct", structName->c_str());
}
}
@@ -4361,13 +4382,11 @@
&fnCandidate->getReturnType());
if (callNode == nullptr)
{
- std::stringstream extraInfoStream;
- extraInfoStream
- << "built in unary operator function. Type: "
- << static_cast<TIntermTyped *>(paramNode)->getCompleteString();
- std::string extraInfo = extraInfoStream.str();
- error(paramNode->getLine(), " wrong operand type", "Internal Error",
- extraInfo.c_str());
+ std::stringstream reasonStream;
+ reasonStream << "wrong operand type for built in unary function: "
+ << static_cast<TIntermTyped *>(paramNode)->getCompleteString();
+ std::string reason = reasonStream.str();
+ error(paramNode->getLine(), reason.c_str(), "Internal Error");
*fatalError = true;
return nullptr;
}