- User Since
- Dec 19 2016, 5:58 AM (161 w, 4 h)
Mon, Jan 6
Great job, this seems to be progressing nicely! please see my comments inline.
Oct 30 2019
- Remove link reference from commit message
- Remove superfluous semicolons left in the code
- Add capture by reference test
- Add another class to ensure reachability inside function call is independent from each other in the first 2 test cases
Oct 29 2019
Oct 22 2019
Please feel free to add more reviewers.
Aug 14 2019
Nice improvements! It was a good catch that a substantial part of the checker could be cut out. Accurate modelling of CString and related API-s is really important, and this is a good step in that direction.
I would test this on a larger codebase as well before committing, but LGTM otherwise.
@chrish_ericsson_atx Thanks for fixing this! Your help is much appreciated :)
Aug 6 2019
Aug 5 2019
- Remove unused member Limit
- Rebase to current master
Aug 2 2019
Lovely documentation with practical use-cases!
I left a few inline remarks.
Also wouldn't it be nice to have a section which introduces the -ast-merge command-line option. It would be helpful to see an example where a PCH is dumped and merged into another TU. You could mention, that this can be used to debug ASTImporter functionality.
Aug 1 2019
Specify the exact meaning of successful storage
Jul 30 2019
Updated the revision.
I find this solution definitely more compact with responsibilities more separated. One more thing that comes to mind is that maybe the whole explicit passing of CTUDir and IndexName arguments is a bit verbose. What are your thoughts about these being injected earlier than the query operations (eg.: in getASTUnitForFunction and getFileForFunction)?
- Merge RAII class
- Update comments
Jul 24 2019
Thanks for pointing out these issues. Most of them are agreed. Merging the RAII counter with the threshold checker class, however, does not seem like a good decision for me. What would be the benefits of merging the two?
Jul 23 2019
Too much autoformat fixed
Refactor functionality into local classes
32f220c5fbe5 is the more sophisticated solution to the problem.
There are multiple new additions to stream formatters, and this patch is way too old. I may consider creating a new checker like this with an extended feature set later.
- Add implementation rationale, fix forgotten build-step
Jul 15 2019
Jul 8 2019
Jul 5 2019
Jul 3 2019
I have applied the fixes, which seemed fitting. Also see my reasoning behind the command-prefix implementation inline.
- Remove redundant parameter handling
- Pass env keyword argument to every ShellCommand step
- Use RemoveDirectory where applicable
@gkistanova Your review is very much appreciated!
I have answered the inline comments, and I am currently applying the suggested fixes. After short testing, I will update this revision.
Jul 2 2019
Missed arcanist diff. Disregard this revision.
Rebase onto current master
May 30 2019
Friendly ping @gkistanova. What do you think about this modification? Also, should we add our buildslave to the silent (if I remember correctly it is called staging), or the reporting buildmaster? Thanks!
May 20 2019
May 13 2019
Revert unnecessary clang-formating of AnalysisConsumer.cpp
I could greatly simplify the API of getCrossTUDefinition by injecting the threshold value in the constructor. A minor drawback with this approach is that testing code is a bit more complicated, as I had to patch the CompilerInstance to override the AnalyzerOption value for CTUImportThreshold. Thanks for the suggestion!
Apply review suggestions by Xazax
Mar 31 2019
Mar 30 2019
Just have a bit more context, I have the following information from a debug session at the execution point of the unreachable:
Mar 28 2019
This issue came up during the generation BugReports of BugPaths containing macro-expansions, where the spelling location and expansion locations were in different files.
With this change, we make such SourceLocations comparable by their FileIDs.
Updated option handling location to use AnalyzerOptions instead of CC1
Mar 25 2019
During CTU analysis, not only StructuralEquivalence but the main implementation of ASTImporter can also emit ODR-related diagnostics. After some consideration, I have chosen not to make these dependent on a switch, as ASTImporter is not nearly as widely used as StructuralEquivalence (thru Sema), and it would also complicate the code unnecessarily.
What do you think?
- Revert member order to original
Mar 7 2019
Mar 4 2019
This revision is the alternative of the abandoned D55646.
Now Sema uses the old behavior of emitting ODR errors while merging and importing is done in a more lenient way.
Feb 14 2019
I am creating a new revision that keeps the old handling of ODR violations the same when used by parts of the Sema, but emit warnings when used by the ASTImporter.
I am going to abandon this modification, as setting ODR violations as warnings, seems like a change that could have unforeseen consequences.
Feb 6 2019
Remove unnecessary ToTU variable from test case.
Feb 5 2019
Jan 8 2019
Thank you for the input! ODR violations being warnings would be beneficial from the code maintenance point of view, as we would not have to resort to duplicate some (if not most) of them as errors. There is also a flexibility advantage in the diagnostics, as warnings can be propagated to error level or suppressed, whereas errors cannot be "demoted" (AFAIK). So while I am not opposed to providing an error AND a warning for these kinds of violations, if the SEMA or other modules do not need them to be explicitly defined as errors (for I don't know... tooling support or other reasons), then it would be cleaner to only have warnings for these.
Dec 13 2018
In order to be able to handle ODR-related diagnostics with command line options, these diagnostics were moved from Error category to Warning. What are your thoughts?
Dec 10 2018
In my opinion, after migrating relevant test cases from D33826, this is ready.
Nov 5 2018
Thanks for your reviews, although I haven't been active for some time now.
I personally do not have commit rights, so could someone else take care of it?
Aug 22 2017
I have implemented the std::transform. The previous version used std::for_each because the iterator for enum declarations was not a random access iterator, but it turned out that I can solve this problem via std::distance. Thanks for sticking to your opinion on this one, because of it I could learn something new.
Aug 20 2017
Ping. @NoQ would you please have a look? Thanks!
Aug 3 2017
As for the the loss of precision problem, in the special case of char the size of char is known. However does the analysis have the necessary information in this stage to know the size of an int for example? I found bit-width specifying information in the llvm::Type class which is used in the code generation phase. It could be done by checking on a per type basis, but then again, it could possibly lead to false positives. Correct me if I am wrong.
Applied most of the suggested changes, thanks for all the insights!
Jul 24 2017
After experimentation the following AST difference between the mock and the standard library implementation still stands (which necessitates the special handling of the complex manipulators). Example:
Fixed the naming convention issues. Also applied the suggested modifications inside the overridden checker method.