steakhal planned changes to this revision.
steakhal added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:20-21
#include "clang/Basic/Version.h"
#include "clang/CrossTU/CrossTranslationUnit.h"
#include "clang/Frontend/ASTUnit.h"
#include "clang/Lex/Preprocessor.h"
----------------
NoQ wrote:
> Will these two includes be ultimately removed? Like, even in the CTU case?
I think `PlistDiagnostics` can depend on `CTU`.
`ASTUnit` is a different thing though. I think I just accidentally left that
include.
I will remove the `#include "clang/Frontend/ASTUnit.h"` line.
---
What do you mean by the 'CTU case'?
================
Comment at:
clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist:148
<key>name</key><string>SET_PTR_VAR_TO_NULL</string>
- <key>expansion</key><string>ptr = 0</string>
+ <key>expansion</key><string>ptr =0</string>
</dict>
----------------
NoQ wrote:
> steakhal wrote:
> > xazax.hun wrote:
> > > martong wrote:
> > > > martong wrote:
> > > > > I wonder how much added value do we have with these huge and clumsy
> > > > > plist files... We already have the concise unittests, which are quite
> > > > > self explanatory. I'd just simply delete these plist files.
> > > > Perhaps in the test cpp file we should just execute a FileCheck for the
> > > > expansions. We are totally not interested to check the whole contents
> > > > of the plist, we are interested only to see if there were expansions.
> > > We do need some plist tests to ensure that the correct plist format is
> > > emitted. How much of those do we need might be up for debate.
> > It's certainly a pain to keep all the locations in sync with the code.
> I'm absolutely in favor of not testing the //entire// plist files whenever
> possible. Just keep some minimal context available so that to not
> accidentally match the wrong test. Yes we obviously need some tests for the
> entire plist files but we already have a lot of that.
Ok, I will remove these files.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93224/new/
https://reviews.llvm.org/D93224
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
