presubmit check for FILE_PATH_LITERAL should ignore commented-out code |
||
Issue description
I am getting this presubmit error:
Errors:
* Please assume FilePath::CharType is char ( crbug.com/870621 ):
* metrics/cumulative_metrics.h, line 12 has FILE_PATH_LITERAL
where line 12 is the second-to-last line below:
// CumulativeMetrics helps maintain and report "accumulated" quantities, for
// instance how much data has been sent over WiFi and LTE in a day. Here's
// roughly how a continuously running daemon would do that:
//
// {
// // initialization, at daemon startup
// ...
// base::FilePath backing_dir = FILE_PATH_LITERAL("/var/lib/metrics/shill");
// std::vector<std::string> stat_names = {"wifi", "cellular", "total"};
,
Aug 16
Oh I see. But the error message could be better, no? I thought the check was confused. I'll fix the comment, but I'll let others decide on closing this.
,
Aug 16
the error message seems accurate to me ... don't use FILE_PATH_LITERAL, and line 12 does indeed use FILE_PATH_LITERAL suggestion on rephrasing ?
,
Aug 16
All right---first of all, when I wrote the comment early on, I was misunderstanding the meaning of FILE_PATH_LITERAL, assuming that it would return a FilePath. But this would be caught by the compiler. So, knowing that FILE_PATH_LITERAL instead is used the string passed to a FilePath constructor, maybe you mean to say "FILE_PATH_LITERAL is not needed because it has no effect here" or something similar. The message "file x, line y has FILE_PATH_LITERAL" has no information because I can see that myself.
,
Aug 17
the explanation is the first line (why): * Please assume FilePath::CharType is char ( crbug.com/870621 ): the second line is explaining what exactly the check is flagging (what): * metrics/cumulative_metrics.h, line 12 has FILE_PATH_LITERAL if we only showed one of the lines, it'd be confusing why it's wrong or what is wrong.
,
Aug 17
semenzato@, thank you for the feedback. The error message "Please assume FilePath::CharType is char ( crbug.com/870621 )" may not be very clear, but I chose the wording because the message is used for multiple cases: FILEPATH_REGEXP = re.compile('|'.join( [r'(?:base::)?FilePath::(?:Char|String|StringPiece)Type', r'(?:base::)?FilePath::FromUTF8Unsafe', r'AsUTF8Unsafe', r'FILE_PATH_LITERAL'])) I included " crbug.com/870621 " in the message for the context. Let me close the bug for now since I guess the error message is good enough. |
||
►
Sign in to add a comment |
||
Comment 1 by vapier@chromium.org
, Aug 16mmm i think the presumit is OK and the example code should be fixed to not use FILE_PATH_LITERAL. it should instead read: base::FilePath backing_dir("/var/lib/metrics/shill");