Swift: extract diagnostics #11101
Swift: extract diagnostics #11101
Conversation
AlexDenisov
force-pushed
the
alexdenisov/extractor-errors
branch
2 times, most recently
from
8997450
to
7c6aaa8
Compare
| @@ -11,6 +11,7 @@ | |||
| #include "swift/extractor/infra/SwiftTagTraits.h" | |||
| #include "swift/extractor/trap/generated/TrapClasses.h" | |||
| #include "swift/extractor/infra/file/PathHash.h" | |||
There was a problem hiding this comment.
nit: this include is not needed any more I think
|
|
||
| class TrapDomain; | ||
|
|
||
| class SwiftLocationExtractor { |
There was a problem hiding this comment.
nit: as this class has no template member function (
|
|
||
| class TrapDomain; | ||
|
|
||
| class CodeQLDiagnosticsConsumer : public swift::DiagnosticConsumer { |
There was a problem hiding this comment.
nit: shouldn't this be SwiftDiagnosticsConsumer to be consistent with other names?
| // Translate ranges. | ||
| llvm::SmallVector<llvm::SMRange, 2> ranges; | ||
| for (auto R : diagInfo.Ranges) | ||
| ranges.push_back(getRawRange(sourceManager, R)); | ||
|
|
||
| // Translate fix-its. | ||
| llvm::SmallVector<llvm::SMFixIt, 2> fixIts; | ||
| for (const swift::DiagnosticInfo::FixIt& F : diagInfo.FixIts) | ||
| fixIts.push_back(getRawFixIt(sourceManager, F)); |
There was a problem hiding this comment.
ranges and fixIts are not used anywhere, what is this code for? If there is some obscure side effect it should be described with comments
There was a problem hiding this comment.
Good catch, they were used originally, but then I tweaked the diagnostics message and they are not needed anymore.
| if (!maybeFile) { | ||
| std::cerr << "Cannot create invocation trap file: " << target << "\n"; | ||
| abort(); | ||
| } |
There was a problem hiding this comment.
what if for some reason we are calling the swift frontend with exactly the same arguments? Should we really abort in such a case? Also, now that I think about it hashing the command line flags might miss changes to the files provided to the call...
As there is no need to reference this output trap file outside of this very call of the extractor, maybe it'd be better to just use a random new file name every time?
There was a problem hiding this comment.
Yeah, you are right, some random name would work better.
There was a problem hiding this comment.
as discussed on chat, probably timestamp + getpid is a good fit for the trap target. Also, maybe use invocations as a directory containing the traps rather than as a filename prefix?
AlexDenisov
force-pushed
the
alexdenisov/extractor-errors
branch
from
92c432d
to
42004d9
Compare
AlexDenisov
force-pushed
the
alexdenisov/extractor-errors
branch
from
f98f586
to
3f2f328
Compare
No description provided.