Skip to content

Commit

Permalink
C++: Report MSVC errors in correct locations.
Browse files Browse the repository at this point in the history
  • Loading branch information
@petervdonovan
petervdonovan committed Jan 6, 2022
1 parent d3f3e06 commit 01a69c2
Show file tree
Hide file tree
Showing 3 changed files with 78 additions and 48 deletions.
35 changes: 22 additions & 13 deletions org.lflang/src/org/lflang/generator/HumanReadableReportingStrategy.java
View file
Expand Up @@ -25,6 +25,8 @@ public class HumanReadableReportingStrategy implements DiagnosticReporting.Strat
private final Pattern labelPattern;
/** The path against which any paths should be resolved. */
private final Path relativeTo;
/** The next line to be processed, or {@code null}. */
private String bufferedLine;

/**
* Instantiates a reporting strategy for lines of
Expand All @@ -41,14 +43,7 @@ public class HumanReadableReportingStrategy implements DiagnosticReporting.Strat
* and "column" groups.
*/
public HumanReadableReportingStrategy(Pattern diagnosticMessagePattern, Pattern labelPattern) {
for (String groupName : new String[]{"path", "line", "column", "message", "severity"}) {
assert diagnosticMessagePattern.pattern().contains(groupName) : String.format(
"Error line patterns must have a named capturing group called %s", groupName
);
}
this.diagnosticMessagePattern = diagnosticMessagePattern;
this.labelPattern = labelPattern;
this.relativeTo = null;
this(diagnosticMessagePattern, labelPattern, null);
}

/**
Expand All @@ -75,12 +70,20 @@ public HumanReadableReportingStrategy(Pattern diagnosticMessagePattern, Pattern
this.diagnosticMessagePattern = diagnosticMessagePattern;
this.labelPattern = labelPattern;
this.relativeTo = relativeTo;
this.bufferedLine = null;
}

@Override
public void report(String validationOutput, ErrorReporter errorReporter, Map<Path, CodeMap> map) {
Iterator<String> it = validationOutput.lines().iterator();
while (it.hasNext()) reportErrorLine(it.next(), it, errorReporter, map);
while (it.hasNext() || bufferedLine != null) {
if (bufferedLine != null) {
reportErrorLine(bufferedLine, it, errorReporter, map);
bufferedLine = null;
} else {
reportErrorLine(it.next(), it, errorReporter, map);
}
}
}

/**
Expand All @@ -97,7 +100,8 @@ private void reportErrorLine(String line, Iterator<String> it, ErrorReporter err
if (matcher.matches()) {
final Path path = Paths.get(matcher.group("path"));
final Position generatedFilePosition = Position.fromOneBased(
Integer.parseInt(matcher.group("line")), Integer.parseInt(matcher.group("column"))
Integer.parseInt(matcher.group("line")),
Integer.parseInt(matcher.group("column") != null ? matcher.group("column") : "0") // FIXME: Unreliable heuristic
);
final String message = DiagnosticReporting.messageOf(
matcher.group("message"), path, generatedFilePosition
Expand All @@ -112,9 +116,13 @@ private void reportErrorLine(String line, Iterator<String> it, ErrorReporter err
// FIXME: Is it desirable for the error to be reported to every single LF file associated
// with the generated file containing the error? Or is it best to be more selective?
Position lfFilePosition = map.adjusted(srcFile, generatedFilePosition);
reportAppropriateRange(
(p0, p1) -> errorReporter.report(severity, message, p0, p1), lfFilePosition, it
);
if (matcher.group("column") != null) {
reportAppropriateRange(
(p0, p1) -> errorReporter.report(severity, message, p0, p1), lfFilePosition, it
);
} else {
errorReporter.report(severity, message, lfFilePosition.getOneBasedLine());
}
}
}
}
Expand Down Expand Up @@ -152,6 +160,7 @@ private void reportAppropriateRange(
}
if (diagnosticMessagePattern.matcher(line).find()) {
failGracefully.apply();
bufferedLine = line;
return;
}
reportAppropriateRange(report, lfFilePosition, it);
Expand Down
2 changes: 2 additions & 0 deletions org.lflang/src/org/lflang/generator/cpp/CppCmakeGenerator.kt
View file
Expand Up @@ -35,6 +35,7 @@ class CppCmakeGenerator(private val targetConfig: TargetConfig, private val file

companion object {
const val includesVarName: String = "TARGET_INCLUDE_DIRECTORIES"
const val compilerIdName: String = "CXX_COMPILER_ID"
}

/** Convert a log level to a severity number understood by the reactor-cpp runtime. */
Expand Down Expand Up @@ -148,6 +149,7 @@ class CppCmakeGenerator(private val targetConfig: TargetConfig, private val file
|get_target_property(TARGET_INCLUDE_DIRECTORIES $S{LF_MAIN_TARGET} INCLUDE_DIRECTORIES)
|list(APPEND TARGET_INCLUDE_DIRECTORIES $S{SOURCE_DIR}/include)
|set(${includesVarName} $S{TARGET_INCLUDE_DIRECTORIES} CACHE STRING "Directories included in the main target." FORCE)
|set(${compilerIdName} $S{CMAKE_CXX_COMPILER_ID} CACHE STRING "The name of the C++ compiler." FORCE)
|
${" |"..(includeFiles?.joinToString("\n") { "include(\"$it\")" } ?: "") }
""".trimMargin()
Expand Down
89 changes: 54 additions & 35 deletions org.lflang/src/org/lflang/generator/cpp/CppValidator.kt
View file
Expand Up @@ -9,6 +9,7 @@ import org.lflang.generator.Validator
import org.lflang.util.LFCommand
import java.io.File
import java.nio.file.Path
import java.nio.file.Paths
import java.util.regex.Pattern

class CppValidator(
Expand All @@ -18,10 +19,8 @@ class CppValidator(
): Validator(errorReporter, codeMaps) {

companion object {
/** This matches the line in the CMake cache that states the C++ standard. */
private val cmakeCxxStandard: Pattern = Pattern.compile("CMAKE_CXX_STANDARD:STRING=(?<cppStandard>.*)")
/** This matches the line in the CMake cache that states the compiler includes for a given target. */
private val cmakeIncludes: Pattern = Pattern.compile("${CppCmakeGenerator.includesVarName}:STRING=(?<includes>.*)")
/** This matches a line in the CMake cache. */
private val cmakeCachedVariable: Pattern = Pattern.compile("(?<name>[\\w_]+):(?<type>[\\w_]+)=(?<value>.*)")

/** This matches a line of error reports from g++. */
private val gxxErrorLine: Pattern = Pattern.compile(
Expand All @@ -32,6 +31,11 @@ class CppValidator(
/** This matches a line of error reports from Clang-Tidy. */
private val clangTidyErrorLine: Pattern = gxxErrorLine
private val clangTidyLabel: Pattern = gxxLabel
/** This matches a line of error reports from MSVC. */
private val msvcErrorLine: Pattern = Pattern.compile(
"(?<path>.+\\.((cc)|(hh)))\\((?<line>\\d+)(,\\s*(?<column>\\d+))?\\)\\s*:.*?(?<severity>(error)|(warning)) [A-Z]+\\d+:\\s*(?<message>.*?)"
)
private val msvcLabel: Pattern = Pattern.compile("(?!)") // MSVC does not use labels, so nothing matches this.
}

private class CppValidationStrategy(
Expand All @@ -54,11 +58,14 @@ class CppValidator(

/**
* [CppValidationStrategyFactory] instances map validator instances to validation strategy instances.
* @param compilerId The CMake compiler ID of the compiler whose output is most similar to the output
* used by the given strategy.
* @param create The function that creates a strategy from a validator.
*/
private enum class CppValidationStrategyFactory(val create: (CppValidator) -> CppValidationStrategy) {
private enum class CppValidationStrategyFactory(val compilerId: String, val create: ((CppValidator) -> CppValidationStrategy)) {

// Note: Clang-tidy is slow (on the order of tens of seconds) for checking C++ files.
CLANG_TIDY({ cppValidator -> CppValidationStrategy(
CLANG_TIDY("Clang", { cppValidator -> CppValidationStrategy(
{ _, _, _ -> },
HumanReadableReportingStrategy(clangTidyErrorLine, clangTidyLabel),
5,
Expand All @@ -68,7 +75,7 @@ class CppValidator(
LFCommand.get("clang-tidy", args, cppValidator.fileConfig.outPath)
}
)}),
GXX({ cppValidator -> CppValidationStrategy(
GXX("GNU", { cppValidator -> CppValidationStrategy(
HumanReadableReportingStrategy(gxxErrorLine, gxxLabel),
{ _, _, _ -> },
1,
Expand All @@ -78,6 +85,19 @@ class CppValidator(
args.add(generatedFile.toString())
LFCommand.get("g++", args, cppValidator.fileConfig.outPath)
}
)}),
MSVC("MSVC", { cppValidator -> CppValidationStrategy(
{ _, _, _ -> },
HumanReadableReportingStrategy(msvcErrorLine, msvcLabel),
3,
{ generatedFile: Path ->
val setUpDeveloperEnvironment: Path = Paths.get(cppValidator.cmakeGeneratorInstance)
.resolve("Common7${File.separator}Tools${File.separator}VsDevCmd.bat")
val args: MutableList<String> = mutableListOf("&", "cl", "/Zs", "/diagnostics:column", "/std:c++${cppValidator.cppStandard}")
cppValidator.includes.forEach { args.add("/I$it") }
args.add(generatedFile.toString())
LFCommand.get(setUpDeveloperEnvironment.toString(), args, cppValidator.fileConfig.outPath)
}
)});
}

Expand All @@ -86,40 +106,39 @@ class CppValidator(
* strategies for the build process carried out by
* CMake and Make.
*/
override fun getBuildReportingStrategies(): Pair<DiagnosticReporting.Strategy, DiagnosticReporting.Strategy>
= Pair(
CppValidationStrategyFactory.GXX.create(this).errorReportingStrategy,
CppValidationStrategyFactory.GXX.create(this).errorReportingStrategy
override fun getBuildReportingStrategies(): Pair<DiagnosticReporting.Strategy, DiagnosticReporting.Strategy> {
val compilerId: String = getFromCache(CppCmakeGenerator.compilerIdName) ?: "GNU" // This is just a guess.
val mostSimilarValidationStrategy = CppValidationStrategyFactory.values().find({ it.compilerId == compilerId })
if (mostSimilarValidationStrategy === null) {
return Pair(DiagnosticReporting.Strategy { _, _, _ -> }, DiagnosticReporting.Strategy { _, _, _ -> })
}
return Pair(
mostSimilarValidationStrategy.create(this).getErrorReportingStrategy(),
mostSimilarValidationStrategy.create(this).getOutputReportingStrategy(),
)
// This is a rather silly function, but it is left as-is because the appropriate reporting strategy
// could in principle be a function of the build process carried out by CMake. It just so happens
// that the compilers that are supported in the current version seem to use the same reporting format,
// so this ends up being a constant function.
}

/** The include directories required by the generated files. */
private val includes: List<String>
get() {
// FIXME: assert cmakeCache.exists()?
if (cmakeCache.exists()) cmakeCache.useLines {
for (line in it) {
val matcher = cmakeIncludes.matcher(line)
if (matcher.matches()) return matcher.group("includes").split(';')
}
private fun getFromCache(variableName: String): String? {
if (cmakeCache.exists()) cmakeCache.useLines {
for (line in it) {
val matcher = cmakeCachedVariable.matcher(line)
if (matcher.matches() && matcher.group("name") == variableName) return matcher.group("value")
}
return listOf()
}
return null
}

/** The include directories required by the generated files. */
private val includes: List<String>
get() = getFromCache(CppCmakeGenerator.includesVarName)?.split(';') ?: listOf()

/** The C++ standard used by the generated files. */
private val cppStandard: String
get() {
if (cmakeCache.exists()) cmakeCache.useLines {
for (line in it) {
val matcher = cmakeCxxStandard.matcher(line)
if (matcher.matches()) return matcher.group("cppStandard")
}
}
return ""
}
private val cppStandard: String?
get() = getFromCache("CMAKE_CXX_STANDARD")

/** The desired instance of the C++ build system. */
private val cmakeGeneratorInstance: String?
get() = getFromCache("CMAKE_GENERATOR_INSTANCE")

/** The CMake cache. */
private val cmakeCache: File = fileConfig.buildPath.resolve("CMakeCache.txt").toFile()
Expand Down

0 comments on commit 01a69c2

Please sign in to comment.