C#: Introduce extractor mode to identify DBs created with codeql test run
#7515
Conversation
Looks nice! I have added some questions/comments and suggestions.
| { | ||
| if (arg == "--verbose") |
There was a problem hiding this comment.
The semantics of the --verbose flag has now been changed by using the base.HandleFlag method. Is this intentional?
| { | ||
| private readonly AssemblyList assemblyList = new AssemblyList(); | ||
|
|
||
| public ExtractorOptions(string[] args) | ||
| { | ||
| Verbosity = Verbosity.Info; | ||
| Threads = System.Environment.ProcessorCount; | ||
| PDB = true; |
There was a problem hiding this comment.
In the CommonOptions the logic for the PDB and CIL flags are connected. I suppose it doesn't matter as we didn't use the CIL flag before in the ExtractorOptions.
| @@ -25,7 +25,7 @@ public override IEnumerable<IExtractionProduct> Contents | |||
| else | |||
| Context.TrapWriter.Archive(TransformedPath, text); | |||
|
|
|||
| yield return Tuples.file_extraction_mode(this, 2); | |||
| yield return Tuples.file_extraction_mode(this, Context.Extractor.Mode | ExtractorMode.Pdb); | |||
There was a problem hiding this comment.
Fantastic that we get rid of the magic number and rely on the flag, but shouldn't this only be ExtractorMode.Pdb?
| @@ -61,7 +61,7 @@ public override void Populate(TextWriter trapFile) | |||
| } | |||
| } | |||
|
|
|||
| trapFile.file_extraction_mode(this, Context.Extractor.Standalone ? 1 : 0); | |||
| trapFile.file_extraction_mode(this, Context.Extractor.Mode); | |||
There was a problem hiding this comment.
Now printing the "entire" mode and not only, whether the it is standalone extraction to the trap file. Is this intentional?
| private readonly ExtractorMode mode; | ||
| public override ExtractorMode Mode => mode; |
There was a problem hiding this comment.
| private readonly ExtractorMode mode; | |
| public override ExtractorMode Mode => mode; | |
| public override ExtractorMode Mode { get; }; |
| mode = ExtractorMode.Standalone; | ||
| if (options.QlTest) | ||
| mode |= ExtractorMode.QlTest; |
There was a problem hiding this comment.
Are we not strict on always using { } for single line if-statements (sometimes this can be overlooked in code-reviews if more statements are added to the if-block)?
| public override bool Standalone => false; | ||
|
|
||
| private readonly ExtractorMode mode; | ||
| public override ExtractorMode Mode => mode; |
There was a problem hiding this comment.
Same comments as for the StandaloneExtractor.
| @@ -37,13 +37,18 @@ public abstract class CommonOptions : ICommandLineOptions | |||
| /// <summary> | |||
| /// Whether to extract PDB information. | |||
| /// </summary> | |||
| public bool PDB { get; private set; } = false; | |||
| public bool PDB { get; set; } = false; | |||
There was a problem hiding this comment.
Maybe consider to use the HandleFlag method to set PDB instead of making PDB mutable (however, this will impact the CIL flag as well - but it seems to be un-unused).
| /// <summary> | ||
| /// Whether extraction is done using `codeql test run`. | ||
| /// </summary> | ||
| public bool QlTest { get; private set; } = false; |
There was a problem hiding this comment.
Excellent!
| predicate isPdbSourceFile() { | ||
| exists(int i | | ||
| file_extraction_mode(this, i) and | ||
| i.bitAnd(2) = 2 |
There was a problem hiding this comment.
So this is needed because the entire "mode" is now used and you need to ensure that the specific flag (in this "2" which corresponds to PDB) is present?
Passing
--qltestto the extractor when run thoughcodeql test runis not done yet, that will be done on an internal follow-up PR.The text was updated successfully, but these errors were encountered: