honggyu.kim added a comment.
Sorry for the late answer.
I would like to write the comment for HTML report comparison first.
> > As I mentioned, I think the simplist way of comparing HTML reports is to
> > have the BugID in each HTML report so that we can just compare HTML reports
> > >whether t
honggyu.kim added a comment.
I would like to also write about bug identification methods.
As I observed the current CmpRuns.py script, the IssueIdentifier is defined as
follows:
def getIssueIdentifier(self) :
id = self.getFileName() + "+"
if 'issue_context' in self._data :
zaks.anna added a comment.
honggyu.kim,
Uniquing HTML reports is out of the scope of this patch and should be discussed
elsewhere (either send a design idea to cfe-dev, send a patch for review, or
file a bugzilla request).
I agree that this patch is a definite improvement to issue identificati
honggyu.kim added a comment.
In http://reviews.llvm.org/D10305#242159, @zaks.anna wrote:
> honggyu.kim,
>
> Uniquing HTML reports is out of the scope of this patch and should be
> discussed elsewhere (either send a design idea to cfe-dev, send a patch for
> review, or file a bugzilla request).
zaks.anna added a comment.
Thanks! Will do.
http://reviews.llvm.org/D10305
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
honggyu.kim added a comment.
Hi Babati,
Can you please rebase this patch based on http://reviews.llvm.org/D12673?
It is just a whitespace cleanup patch and I wrote
http://reviews.llvm.org/D12906 based on this patch after applying whitespace
cleanup at the end of each line.
http://reviews.llvm.o
phillip.power added a comment.
Hi Babati,
We at Sony are interested in this feature so that our tools can suppress
undesirable bug warnings. You described at the end of the summary that you have
thought about introducing new hash calculation algorithms if needed. How do
you expect this to wor
babati added a comment.
Hi
Sorry for the late answer.
> Can you please rebase this patch based on http://reviews.llvm.org/D12673?
Yes, I can. The patch will comes soon.
> How do you expect this to work? i.e. would bug_id_1 always be generated along
> with new improved bug_ids in the same pli
xazax.hun added a comment.
Even if the analzyer no longer generates new hashes it should be easy to
maintain old hash algorithms out of tree.
http://reviews.llvm.org/D10305
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.o
zaks.anna added a comment.
Hi Babati,
As far as I can see, the following comments from June 15th have not been
addressed. It would be good if you could address them in the latest revision.
"I would be interested in either replacing "issue_hash" or adding
"issue_hash_bug_line_content" (or somet
dkrupp added a comment.
Hi,
Regarding testing:
I think we should create a RecursiveASTvistor based "test checker" that matches
every statement and declaration and reports a bug there.
Then we could create a test file similar to what we have in
/tools/clang/test/Analysis/diagnostics/report-issue
zaks.anna added a comment.
Could you address this:
Could you ask on the open source list if people are using "issue_hash" and if
they are Ok with us renaming it.
I'd suggest to suffix each issue hash field with the description of that hash.
For example, we would remove "issue_hash" and replace
xazax.hun marked 9 inline comments as done.
Comment at: lib/StaticAnalyzer/Core/BugId.cpp:29
@@ +28,3 @@
+
+static std::string GetSignature(const FunctionDecl *Target) {
+ if (!Target)
zaks.anna wrote:
> Can/Should we use some existing machinery for this? For exa
zaks.anna added a comment.
> Generating mangled names requires ASTContext which is not available during
> the error reporting. BugReporter does have the ASTContext, so it would not
> be a big change to add it to the DiagnosticConsumers though. And I think the
> mangled name might contain too
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.
In http://reviews.llvm.org/D10305#258671, @zaks.anna wrote:
> > Generating mangled names requires ASTContext which is not available during
> > the error reporting. BugReporter does have the ASTContext, so it would not
>
> >
zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.
Thanks for working on this!
LGTM, Anna.
http://reviews.llvm.org/D10305
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://list
xazax.hun added a comment.
Thank you for the review!
Before committing this I would like to have a policy regarding future changes
and document it inside the IssueHash header.
My proposed policy is the following:
- Do not change the calculation of issue hash unless we have a very good reason
t
xazax.hun added a comment.
In http://reviews.llvm.org/D10305#268777, @phillip.power wrote:
> In http://reviews.llvm.org/D10305#262534, @xazax.hun wrote:
>
> > - Should we require the generation of old hashes once a change is
> > introduced, or should we expect users who rely on old hash to maint
phillip.power added a comment.
In http://reviews.llvm.org/D10305#262534, @xazax.hun wrote:
> - Should we require the generation of old hashes once a change is introduced,
> or should we expect users who rely on old hash to maintain the old hash
> generation as an out of tree patch?
I will lik
phillip.power added a comment.
In http://reviews.llvm.org/D10305#268795, @xazax.hun wrote:
> > How close is "the near future"? I would like to start using the hashing
> > feature in the next couple of weeks. If your checker identification
> > improvements are a long time out, I would like you
zaks.anna added a comment.
> Should we require the generation of old hashes once a change is introduced,
> or should we expect users
> who rely on old hash to maintain the old hash generation as an out of tree
> patch?
We should maintain the old hashes, at least for a while, if there are peo
honggyu.kim added a comment.
Hi Gabor,
I was away from the office for 3 weeks so I couldn't catch up the issues but
you almost completed the patch now.
Thanks for this work!
I just found a tiny problem and described it in the line comment so please have
a look at it.
Comment
xazax.hun closed this revision.
xazax.hun added a comment.
Committed in r251011.
http://reviews.llvm.org/D10305
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
xazax.hun added a comment.
In http://reviews.llvm.org/D10305#272013, @honggyu.kim wrote:
> Hi Gabor,
>
> I was away from the office for 3 weeks so I couldn't catch up the issues but
> you almost completed the patch now.
> Thanks for this work!
>
> I just found a tiny problem and described it in
seaneveson added a comment.
We are working on tools that use the new hash for bug suppression. There seems
to be no way to predict the names of future hashes. We have products (that will
use the bug identification) that are on a different release schedule to our
clang compiler. These tools will
xazax.hun added a comment.
Thank you for pointing this out. I think it would be great to be forward
compatible.
Right now one could use a heuristic such as for every hash the key has the
"hash" string in its name.
This information might not be sufficient, however, since it would be great to
ha
seaneveson added a comment.
In http://reviews.llvm.org/D10305#286119, @xazax.hun wrote:
> A third alternative would be to have both semantic names (containing hash)
> and a number suffix which indicates the ordering.
Yes that would work, but I don't understand the benefit of having the semanti
zaks.anna added a comment.
> This makes forwards compatibility difficult, since there is no way to predict
> the names of future hashes
> (As far as I understand).
Can you describe what you are trying to achieve?
We can agree that all issue hashes start with "issue_hash" prefix. If you find
seaneveson added a comment.
In http://reviews.llvm.org/D10305#286385, @zaks.anna wrote:
> The reason I like names more than the numbers is that we may use different
> solutions for issue hash generation and some users might prefer one over the
> other. It is not necessarily clear which one is t
zaks.anna added a comment.
> Just for the sake of explaining, lets say in 3 subsequent Analyzer releases
> the hashes are called “hash_1”, “hash_2” and “hash_3”.
> In the first release the suppression tool will record hash_1 to suppress a
> warning. Some developers will upgrade to the second
seaneveson added a comment.
> If you have multiple users using a bug suppression system, I would design
> such system using only a single hash version across all users; using a mix
> seems error prone.. Once all of your users upgrade to a version of the
> analyzer where a new hash version is av
babati updated this revision to Diff 31636.
babati added a comment.
Removed filename.
Updated to the latest trunk.
http://reviews.llvm.org/D10305
Files:
include/clang/StaticAnalyzer/Core/BugId.h
lib/StaticAnalyzer/Core/BugId.cpp
lib/StaticAnalyzer/Core/CMakeLists.txt
lib/StaticAnalyzer/
babati added a comment.
We wanted to include the filename without path in the hash. This would allow us
to compare two runs of the same project (located in different directories)
purely based on the hashes.
This gives good enough results when comparing different versions of the same
project, or
honggyu.kim added a comment.
Hi Babati,
I'm sorry for the late appreciation response. I can see the Bug ID in HTML.
I have also tested CmpRuns.py and it works also fine as Anna explained.
CmpRuns.py works great with plist files, but I personally want to compare two
reports with HTML results with
zaks.anna added a comment.
We need a way to test this functionality. One way of testing this would be to
write unit tests. Gabor has already added such tests for the static analyzer.
http://reviews.llvm.org/D10305
___
cfe-commits mailing list
cfe-c
zaks.anna added a comment.
honggyu.kim,
You are right, CmpRuns.py does not work with HTML files.
The list of HTML reports is produced by scan-build and there is no facility
there to search for newly generated reports. It is also not clear that there
should be one HTML file per report. This is
honggyu.kim added a comment.
In http://reviews.llvm.org/D10305#221121, @zaks.anna wrote:
> honggyu.kim,
>
> You are right, CmpRuns.py does not work with HTML files.
>
> The list of HTML reports is produced by scan-build and there is no facility
> there to search for newly generated reports.
Th
zaks.anna requested changes to this revision.
zaks.anna added a comment.
This revision now requires changes to proceed.
This patch needs tests.
We should treat the improvements to HTML uniquing separately from this patch.
> But I think it's not a critical issue as of now and it can be considered
38 matches
Mail list logo
