Refactor the ignore lists

[Evgeniy Stepanov, 2010-09-13 08:58:07]

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6955
thread_sanitizer.cc:6955: struct IgnoreTriple {
add some comments please

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6977
thread_sanitizer.cc:6977: vector<IgnoreTriple> funs;
"funs" seems like a bad name for this

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7089
thread_sanitizer.cc:7089: ReadIgnoreLine(line, "obj:", g_ignore_lists->funs) ||
You can just parse the prefix in ReadIgnoreLine instead of calling it 5 times.

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7135
thread_sanitizer.cc:7135: bool TripleVectorPartMatch(const vector<IgnoreTriple>&
v,
TripleVectorPartialMatch?
TripleVectorMatchKnown?

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7184
thread_sanitizer.cc:7184: return !(TripleVectorMatch(g_ignore_lists->funs_hist,
rtn_name, "*", "*"));
You can not use stars here! It is a string that is matched against a template,
not a template itself.

[kcc, 2010-09-13 10:39:03]

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6991
thread_sanitizer.cc:6991: } else {
} else if (...) {

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7107
thread_sanitizer.cc:7107: cur = ConvertToPlatformIndependentPath(cur);
Do it in CTOR?

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7168
thread_sanitizer.cc:7168: bool ignore =
TripleVectorPartMatch(g_ignore_lists->funs, rtn_name, img_name, file_name) ||
80 lines

[ramosian.glider, 2010-09-14 10:36:09]

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6955
thread_sanitizer.cc:6955: struct IgnoreTriple {
On 2010/09/13 08:58:07, Evgeniy Stepanov wrote:
> add some comments please

Done.

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6977
thread_sanitizer.cc:6977: vector<IgnoreTriple> funs;
On 2010/09/13 08:58:07, Evgeniy Stepanov wrote:
> "funs" seems like a bad name for this
Fixed.

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7089
thread_sanitizer.cc:7089: ReadIgnoreLine(line, "obj:", g_ignore_lists->funs) ||
On 2010/09/13 08:58:07, Evgeniy Stepanov wrote:
> You can just parse the prefix in ReadIgnoreLine instead of calling it 5 times.

Done.

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7135
thread_sanitizer.cc:7135: bool TripleVectorPartMatch(const vector<IgnoreTriple>&
v,
On 2010/09/13 08:58:07, Evgeniy Stepanov wrote:
> TripleVectorPartialMatch?
> TripleVectorMatchKnown?
TripleVectorMatchKnown.

Please note a change to the function's implementation.

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7184
thread_sanitizer.cc:7184: return !(TripleVectorMatch(g_ignore_lists->funs_hist,
rtn_name, "*", "*"));
On 2010/09/13 08:58:07, Evgeniy Stepanov wrote:
> You can not use stars here! It is a string that is matched against a template,
> not a template itself.

Fixed, thanks!

[ramosian.glider, 2010-09-14 10:42:21]

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6991
thread_sanitizer.cc:6991: } else {
On 2010/09/13 10:39:04, kcc wrote:
> } else if (...) {

Refactored, please see ReadIgnoreLine()

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7107
thread_sanitizer.cc:7107: cur = ConvertToPlatformIndependentPath(cur);
On 2010/09/13 10:39:04, kcc wrote:
> Do it in CTOR? 

Done.

http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7168
thread_sanitizer.cc:7168: bool ignore =
TripleVectorPartMatch(g_ignore_lists->funs, rtn_name, img_name, file_name) ||
On 2010/09/13 10:39:04, kcc wrote:
> 80 lines

Done.

[Evgeniy Stepanov, 2010-09-14 10:48:54]

http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6972
thread_sanitizer.cc:6972: obj = ConvertToPlatformIndependentPath(iobj);
ConvertToPlatformIndependentPath is only applied to file paths

http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6973
thread_sanitizer.cc:6973: file = ConvertToPlatformIndependentPath(file);
ifile

[ramosian.glider, 2010-09-14 10:53:39]

http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6972
thread_sanitizer.cc:6972: obj = ConvertToPlatformIndependentPath(iobj);
On 2010/09/14 10:48:59, Evgeniy Stepanov wrote:
> ConvertToPlatformIndependentPath is only applied to file paths

It was a mistake, I guess. Objects are file names, too.

http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6973
thread_sanitizer.cc:6973: file = ConvertToPlatformIndependentPath(file);
On 2010/09/14 10:48:59, Evgeniy Stepanov wrote:
> ifile
Done.

[timurrrr_at_google_com, 2010-09-14 12:33:20]

http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7002
thread_sanitizer.cc:7002: if (input_line.find("src:") == 0) {
I'd replace something with a macro or a function.

http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7134
thread_sanitizer.cc:7134: (file.size() == 0 || StringMatch(v[i].file, file)))
return true;
"return true" on a separate line, probably with {}

[ramosian.glider, 2010-09-14 12:55:14]

http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7002
thread_sanitizer.cc:7002: if (input_line.find("src:") == 0) {
On 2010/09/14 12:33:20, timurrrr_at_google_com wrote:
> I'd replace something with a macro or a function.

Done.

http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7134
thread_sanitizer.cc:7134: (file.size() == 0 || StringMatch(v[i].file, file)))
return true;
On 2010/09/14 12:33:20, timurrrr_at_google_com wrote:
> "return true" on a separate line, probably with {}

Done.

[kcc, 2010-09-15 08:12:40]

http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc#newcode6997
thread_sanitizer.cc:6997: /* OUT */ string &output) {
string *

http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc#newcode7169
thread_sanitizer.cc:7169: TripleVectorMatchKnown(g_ignore_lists->ignores_hist,
wtf?

[ramosian.glider, 2010-09-15 08:17:35]

http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc#newcode6997
thread_sanitizer.cc:6997: /* OUT */ string &output) {
On 2010/09/15 08:12:41, kcc wrote:
> string *

Fixed

[timurrrr_at_google_com, 2010-09-15 10:51:38]

General comment:
please run the old and new binaries with --debug_phase=ignores on
Mac/media_unittests and compare the logs

http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode6972
thread_sanitizer.cc:6972: file = ConvertToPlatformIndependentPath(ifile);
I'd add an assert that all three params can't be "*" at the same time.

http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode6996
thread_sanitizer.cc:6996: bool CutStringPrefix(const string &input, const string
&prefix,
CutStringPrefixIfPresent ?

http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode7135
thread_sanitizer.cc:7135: if ((fun.size() == 0 || StringMatch(v[i].fun, fun)) &&
Possible performance issue: default value for 2 of 3 vars are "*", not "".

[ramosian.glider, 2010-09-15 11:02:08]

The third comment was fixed too, but a server error occured while sending that
comment

http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode6972
thread_sanitizer.cc:6972: file = ConvertToPlatformIndependentPath(ifile);
On 2010/09/15 10:51:38, timurrrr_at_google_com wrote:
> I'd add an assert that all three params can't be "*" at the same time.

Done.

http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode7135
thread_sanitizer.cc:7135: if ((fun.size() == 0 || StringMatch(v[i].fun, fun)) &&
On 2010/09/15 10:51:38, timurrrr_at_google_com wrote:
> Possible performance issue: default value for 2 of 3 vars are "*", not "".
Discussed offline

[timurrrr_at_google_com, 2010-09-15 11:05:11]

http://codereview.appspot.com/2127047/diff/25001/thread_sanitizer.cc
File thread_sanitizer.cc (right):

http://codereview.appspot.com/2127047/diff/25001/thread_sanitizer.cc#newcode6973
thread_sanitizer.cc:6973: CHECK(!((ifun == iobj) && (iobj == ifile)));
wrong condition (discussed offline)

[ramosian.glider, 2010-09-18 06:57:32]

I've fixed the matcher routine after running the tests, eerything is ok now.
Going to land the CL on Monday