|
|
Created:
4 years, 1 month ago by Ramin Halavati Modified:
3 years, 8 months ago CC:
chromium-reviews, asanka Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTool added to extract network traffic annotations.
This is a clang tool that extracts all occurrences of network traffic annotattions and produces a report of annotated locations and text.
BUG=656607
Review-Url: https://codereview.chromium.org/2448133006
Cr-Commit-Position: refs/heads/master@{#466281}
Committed: https://chromium.googlesource.com/chromium/src/+/3051a40f7c5567d6bbee847227e7a31880fad143
Patch Set 1 #
Total comments: 84
Patch Set 2 : Comments addressed. #
Total comments: 22
Patch Set 3 : Comments addressed. #Patch Set 4 : nits #Patch Set 5 : nits + bug fixed when a directory was specified for extracted partial files. #Patch Set 6 : Windows compatibility. #Patch Set 7 : DEPS file corrected. #Patch Set 8 : Windows running steps reduced. #Patch Set 9 : nits #Patch Set 10 : nits #
Total comments: 8
Patch Set 11 : nits. #Patch Set 12 : Resolving protobuf lite and full conflict #Patch Set 13 : nits #Patch Set 14 : nits #Patch Set 15 : minor bug fixed. #
Total comments: 1
Patch Set 16 : nits #Patch Set 17 : Updated due to protobuf changes #Patch Set 18 : Missing files added. #Patch Set 19 : merged. #Patch Set 20 : nits #Patch Set 21 : nits #
Total comments: 44
Patch Set 22 : Comments addressed. #
Total comments: 6
Patch Set 23 : Compatibility issues with changes in proto and net/ files solved. #Patch Set 24 : nits #
Total comments: 62
Patch Set 25 : Comments addressed. #
Total comments: 16
Patch Set 26 : Extractor simplified. #Patch Set 27 : Minor bug fixed. #Patch Set 28 : Windows compatibility issues fixed. #Patch Set 29 : Clang tool comments addressed. #Patch Set 30 : nits #Patch Set 31 : Tests added. #Patch Set 32 : nits #
Total comments: 53
Patch Set 33 : Comments addressed. #Patch Set 34 : nits #
Total comments: 10
Patch Set 35 : Comments addressed More Windows Corrections. #
Total comments: 14
Patch Set 36 : Clang tool updated. #
Total comments: 14
Patch Set 37 : Clang tool updated. #
Total comments: 4
Patch Set 38 : Clang tool updated. #Patch Set 39 : nits #Patch Set 40 : Auditor replaced. #Patch Set 41 : nits #Patch Set 42 : DEPS updated. #Patch Set 43 : Protobuf preparation separated. #Patch Set 44 : Windows compatibility update. #
Total comments: 20
Patch Set 45 : Comments addressed. #Messages
Total messages: 50 (14 generated)
rhalavati@chromium.org changed reviewers: + battre@chromium.org
Please review this.
https://codereview.chromium.org/2448133006/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2448133006/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:184: "//tools/traffic_annotation/*", Turned out that this is still problematic and checkdeps complains. Please add a TODO. We might need to turn this into a component. I don't know. https://codereview.chromium.org/2448133006/diff/1/tools/clang/scripts/run_too... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/2448133006/diff/1/tools/clang/scripts/run_too... tools/clang/scripts/run_tool.py:307: help='parameters passed to the tool') please fix indentation https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:17: //--> TODO: #include "../../traffic_annotation/traffic_annotation.pb.h" remove? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:24: // Structure to collect instances. instances of what? Can you add more comments to these data structures? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:25: typedef struct Collector { I think that in C++ we don't need to "typedef struct" anymore. struct Collector {}; https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:27: Instance() { Can you use an initializer list? Instance() : is_direct_call(false), transitive_parameter(false), variable_reference(nullptr) {} https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:30: variable_reference = NULL; nullptr https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:36: int line_number; line number is not default initialized. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:44: // Annotation content Comments should end with period. (please also check below) https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:65: std::vector<Instance> variable_definitions, calls; one variable definition per line. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:97: source_manager.getCharacterData(start)); can you also write std::string output(source_manager.getCharacterData(start), source_manager.getCharacterData(end))? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:100: if (output == "R") {} around multiline blocks. Same in the next line. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:151: if (parents.size() == 1) { Add a comment what having a single parent means? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:162: void GetOccuranceLocation(const T* token, typo: occurr*e*nce What's the difference between an occurrence and an instance? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:171: class TheCallback : public MatchFinder::MatchCallback { Description of the responsibility of this class? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:173: explicit TheCallback(Collector* collector) : collector_(collector) {} ~TheCallback() override = default; https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:177: result.Nodes.getNodeAs<clang::VarDecl>("annotation_variable")) also {} if the is statement spans more than one line https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:204: instance.annotation.unique_id.empty() && instance.error.empty()) {} https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:260: // TODO store pointer Delete the todo? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:264: // NRA class keeps the call back function and sets the matchers. Please don't use abbreviations. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:265: class NetworkRequestAuditor { Rename this to NetworkAnnotationTagExtractor? Then TheCallback could be called NetworkAnnotationTagCallback. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:271: // Find variables defined as Annotation . What does "Find variables defined as Annotation" mean? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:309: kOutputSpecifier.length())) { if (argv[i] == kOutputSpecifier) does the same thing, right? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:332: for (auto& c : collector.calls) { const auto& c? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:335: for (auto& v : collector.variable_definitions) {} const auto& ? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:355: output_dir.c_str(), s.c_str(), c.location.line_number); std::string filename = output_dir + "/" + s + "(" + std::to_string(c.location.line_number + ").txt"; https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:357: FILE* file = fopen(file_name, "wt"); how about staying C++ here and using std::ofstream? https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... File tools/traffic_annotation/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:5: // See README.txt for information and build instructions. Please add this. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:25: Instance() { use initializer list? https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:32: uint hash_code; I think that C++ makes no definition of the size of an uint. This should be a fixed size integer (also in the other CL) https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:33: enum { ok, duplicate, undefined } state; See Style guide on the naming of enums. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:71: msg_text->clear(); https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:73: std::string temp; std::string line;? https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:137: // Checks to see if two unique ids have similar hash codes. This function mutates the instances. That should be described and reflected in the function. MarkHashCollisions()? https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:140: for (auto& instance : *instances) {} https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:142: auto p = hashed_ids.find(instance.hash_code); what is p? --> Too short variable name. Maybe "iter"? https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:158: for (auto& instance : instances) { const https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:228: build_dir.value().c_str()); Unless you create a scoped temp directory, the directory does not get deleted on shutdown. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:249: build_dir, extracted_files_dir, path_filters)) {} https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:257: if (instances.size()) { if (!instances.empty()) { https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:265: if (!summary_file.empty()) {} https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:267: command_line.GetSwitchValueASCII("summary_file"))) {}
All comments addressed. Please review. https://codereview.chromium.org/2448133006/diff/1/third_party/protobuf/BUILD.gn File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2448133006/diff/1/third_party/protobuf/BUILD.... third_party/protobuf/BUILD.gn:184: "//tools/traffic_annotation/*", On 2016/10/26 14:29:18, battre wrote: > Turned out that this is still problematic and checkdeps complains. Please add a > TODO. We might need to turn this into a component. I don't know. Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/scripts/run_too... File tools/clang/scripts/run_tool.py (right): https://codereview.chromium.org/2448133006/diff/1/tools/clang/scripts/run_too... tools/clang/scripts/run_tool.py:307: help='parameters passed to the tool') On 2016/10/26 14:29:18, battre wrote: > please fix indentation Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:17: //--> TODO: #include "../../traffic_annotation/traffic_annotation.pb.h" On 2016/10/26 14:29:18, battre wrote: > remove? I think if we want to add protobuf to clang tool in future, we would need to modify clang's build scripts and most probably it would not be a clean job. So if you are OK with current approach, we can remove this reminder. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:24: // Structure to collect instances. On 2016/10/26 14:29:19, battre wrote: > instances of what? Can you add more comments to these data structures? Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:25: typedef struct Collector { On 2016/10/26 14:29:18, battre wrote: > I think that in C++ we don't need to "typedef struct" anymore. > > struct Collector {}; Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:27: Instance() { On 2016/10/26 14:29:18, battre wrote: > Can you use an initializer list? > > Instance() : is_direct_call(false), transitive_parameter(false), > variable_reference(nullptr) {} Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:30: variable_reference = NULL; On 2016/10/26 14:29:19, battre wrote: > nullptr Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:36: int line_number; On 2016/10/26 14:29:19, battre wrote: > line number is not default initialized. Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:44: // Annotation content On 2016/10/26 14:29:18, battre wrote: > Comments should end with period. (please also check below) Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:65: std::vector<Instance> variable_definitions, calls; On 2016/10/26 14:29:19, battre wrote: > one variable definition per line. Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:97: source_manager.getCharacterData(start)); On 2016/10/26 14:29:19, battre wrote: > can you also write > std::string output(source_manager.getCharacterData(start), > source_manager.getCharacterData(end))? Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:100: if (output == "R") On 2016/10/26 14:29:19, battre wrote: > {} around multiline blocks. Same in the next line. Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:151: if (parents.size() == 1) { On 2016/10/26 14:29:18, battre wrote: > Add a comment what having a single parent means? I did not find a conclusive description on this and also did not encounter any case where getParents() returns other than 1 parent. Hence I wrote it this way so that if we encounter any different case in future, we can come back to it. Also as it is clang code, we cannot our normal assertion, log and error commands. Added a TODO comment to come back to it later. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:162: void GetOccuranceLocation(const T* token, On 2016/10/26 14:29:19, battre wrote: > typo: occurr*e*nce > > What's the difference between an occurrence and an instance? Renamed it to GetLocation as it can be called generally for any sort of token. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:171: class TheCallback : public MatchFinder::MatchCallback { On 2016/10/26 14:29:19, battre wrote: > Description of the responsibility of this class? Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:173: explicit TheCallback(Collector* collector) : collector_(collector) {} On 2016/10/26 14:29:18, battre wrote: > ~TheCallback() override = default; Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:177: result.Nodes.getNodeAs<clang::VarDecl>("annotation_variable")) On 2016/10/26 14:29:19, battre wrote: > also {} if the is statement spans more than one line Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:204: instance.annotation.unique_id.empty() && instance.error.empty()) On 2016/10/26 14:29:19, battre wrote: > {} Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:260: // TODO store pointer On 2016/10/26 14:29:19, battre wrote: > Delete the todo? Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:264: // NRA class keeps the call back function and sets the matchers. On 2016/10/26 14:29:18, battre wrote: > Please don't use abbreviations. Done. The class is totally removed and replaced by a single function for simplicity. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:265: class NetworkRequestAuditor { On 2016/10/26 14:29:19, battre wrote: > Rename this to NetworkAnnotationTagExtractor? Then TheCallback could be called > NetworkAnnotationTagCallback. The class is totally removed and replaced by a single function for simplicity. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:271: // Find variables defined as Annotation On 2016/10/26 14:29:19, battre wrote: > . > > What does "Find variables defined as Annotation" mean? Comments changed. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:309: kOutputSpecifier.length())) { On 2016/10/26 14:29:19, battre wrote: > if (argv[i] == kOutputSpecifier) does the same thing, right? argv[i] parameter includes the specifier and the path (e.g. output_dir=~/temp), and we need to check if it's prefix matches kOutSpecifier. Is there a neater way to do that? https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:332: for (auto& c : collector.calls) { On 2016/10/26 14:29:19, battre wrote: > const auto& c? the annotation field of iterators may be changed 5 lines below, hence can't use const. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:335: for (auto& v : collector.variable_definitions) On 2016/10/26 14:29:19, battre wrote: > {} > > const auto& ? Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:355: output_dir.c_str(), s.c_str(), c.location.line_number); On 2016/10/26 14:29:18, battre wrote: > std::string filename = output_dir + "/" + s + "(" + > std::to_string(c.location.line_number + ").txt"; Done. https://codereview.chromium.org/2448133006/diff/1/tools/clang/traffic_annotat... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:357: FILE* file = fopen(file_name, "wt"); On 2016/10/26 14:29:18, battre wrote: > how about staying C++ here and using std::ofstream? Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... File tools/traffic_annotation/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:5: // See README.txt for information and build instructions. On 2016/10/26 14:29:20, battre wrote: > Please add this. Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:25: Instance() { On 2016/10/26 14:29:19, battre wrote: > use initializer list? Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:32: uint hash_code; On 2016/10/26 14:29:20, battre wrote: > I think that C++ makes no definition of the size of an uint. This should be a > fixed size integer (also in the other CL) In the other CL "unsigned" type is returned by several clang function, hence I used it. I replaced uint with unsigned int, which is used by our hash generation function. Is it OK? https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:33: enum { ok, duplicate, undefined } state; On 2016/10/26 14:29:19, battre wrote: > See Style guide on the naming of enums. Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:71: On 2016/10/26 14:29:19, battre wrote: > msg_text->clear(); Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:73: std::string temp; On 2016/10/26 14:29:20, battre wrote: > std::string line;? Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:137: // Checks to see if two unique ids have similar hash codes. On 2016/10/26 14:29:20, battre wrote: > This function mutates the instances. That should be described and reflected in > the function. > > MarkHashCollisions()? Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:140: for (auto& instance : *instances) On 2016/10/26 14:29:20, battre wrote: > {} Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:142: auto p = hashed_ids.find(instance.hash_code); On 2016/10/26 14:29:20, battre wrote: > what is p? --> Too short variable name. Maybe "iter"? Renamed to match. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:158: for (auto& instance : instances) { On 2016/10/26 14:29:19, battre wrote: > const Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:228: build_dir.value().c_str()); On 2016/10/26 14:29:20, battre wrote: > Unless you create a scoped temp directory, the directory does not get deleted on > shutdown. Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:249: build_dir, extracted_files_dir, path_filters)) On 2016/10/26 14:29:19, battre wrote: > {} Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:257: if (instances.size()) { On 2016/10/26 14:29:20, battre wrote: > if (!instances.empty()) { Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:265: if (!summary_file.empty()) On 2016/10/26 14:29:19, battre wrote: > {} Done. https://codereview.chromium.org/2448133006/diff/1/tools/traffic_annotation/tr... tools/traffic_annotation/traffic_annotation_auditor.cc:267: command_line.GetSwitchValueASCII("summary_file"))) On 2016/10/26 14:29:20, battre wrote: > {} Done.
Nice. A few more minor things. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:4: Do you want to make some comments here what this program is about? https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:29: struct Instance { What do you think of moving this struct out of Collector? https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:41: std::string function_name; Can you add examples for this and object_name? // Name of the function including this instance. // e.g. ::Foo::my_function(void*) (does it look like that?) https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:81: // Returns the text of a given statement or subclass. Can you give an example of what this means? https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:85: // Get text range . https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:101: source_manager.getCharacterData(start)); std::string also has a constructor template <class InputIterator> string (InputIterator first, InputIterator last); I thought that you could simplify this further to std::string output(source_manager.getCharacterData(start), source_manager.getCharacterData(end)); https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:137: // Returns the function that includes the given clang::Stmt. Example? If token is a statement like "...", the function returns "...". If it is a declaration like ... I guess my problem is that I am not an expert with Clang tools but other reviewers may suffer from that issue as well. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:140: auto parents = result.Context->getParents(*token); Use DynTypedNodeList instead of auto here? https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:168: // Finds file name and line number of the given token and sets the. sets the $WORD_MISSING https://codereview.chromium.org/2448133006/diff/20001/tools/traffic_annotatio... File tools/traffic_annotation/README.txt (right): https://codereview.chromium.org/2448133006/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/README.txt:2: from chromium source code and collects and summarizes it's outputs. s/it's/its/ https://codereview.chromium.org/2448133006/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/README.txt:28: --summery_file=report.txt --summary_file
Comments addressed, please review. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:4: On 2016/10/27 11:49:06, battre wrote: > Do you want to make some comments here what this program is about? Done. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:29: struct Instance { On 2016/10/27 11:49:06, battre wrote: > What do you think of moving this struct out of Collector? Done. Also renamed it to NetworkAnnotationInstance. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:41: std::string function_name; On 2016/10/27 11:49:06, battre wrote: > Can you add examples for this and object_name? > > // Name of the function including this instance. > // e.g. ::Foo::my_function(void*) > > (does it look like that?) Done. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:81: // Returns the text of a given statement or subclass. On 2016/10/27 11:49:06, battre wrote: > Can you give an example of what this means? Done, but mostly updated the comment. A clang::Stmt can refer to any statement of the AST and this function extracts its source code. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:85: // Get text range On 2016/10/27 11:49:05, battre wrote: > . Done. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:101: source_manager.getCharacterData(start)); On 2016/10/27 11:49:06, battre wrote: > std::string also has a constructor template <class InputIterator> > string (InputIterator first, InputIterator last); > > I thought that you could simplify this further to > std::string output(source_manager.getCharacterData(start), > source_manager.getCharacterData(end)); Done. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:137: // Returns the function that includes the given clang::Stmt. On 2016/10/27 11:49:06, battre wrote: > Example? If token is a statement like "...", the function returns "...". If it > is a declaration like ... > > I guess my problem is that I am not an expert with Clang tools but other > reviewers may suffer from that issue as well. Done. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:140: auto parents = result.Context->getParents(*token); On 2016/10/27 11:49:06, battre wrote: > Use DynTypedNodeList instead of auto here? Done. https://codereview.chromium.org/2448133006/diff/20001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:168: // Finds file name and line number of the given token and sets the. On 2016/10/27 11:49:06, battre wrote: > sets the $WORD_MISSING Done. https://codereview.chromium.org/2448133006/diff/20001/tools/traffic_annotatio... File tools/traffic_annotation/README.txt (right): https://codereview.chromium.org/2448133006/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/README.txt:2: from chromium source code and collects and summarizes it's outputs. On 2016/10/27 11:49:06, battre wrote: > s/it's/its/ Done. https://codereview.chromium.org/2448133006/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/README.txt:28: --summery_file=report.txt On 2016/10/27 11:49:06, battre wrote: > --summary_file Done, also done in program's help text.
Comments addressed, please review.
The code is now fully Windows compatible with just one required manual modification of the ninja build file which will be attended to after we finalize how to use full protobuf.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
LGTM for third_party/protobuf OWNERS. https://codereview.chromium.org/2448133006/diff/180001/third_party/protobuf/B... File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2448133006/diff/180001/third_party/protobuf/B... third_party/protobuf/BUILD.gn:183: # Traffic_annotation is not part of the Chrome build. Nit: Maybe clearer: "The traffic_annotation tool is not part of Chrome itself, and needs to parse human-readable protobufs." https://codereview.chromium.org/2448133006/diff/180001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.txt (right): https://codereview.chromium.org/2448133006/diff/180001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.txt:12: command prompt and run "depot_tools\win_toolchain\vs_fies\ Typo (fies). But assume people are using a command prompt that has the MSVC tools set up in the environment, as that's what our setup steps will produce, and not doing this makes all kinds of stuff break; so just omit this step. Given that the second step is just the same as the Linux instructions with the addition of "python" in front, I'd omit that too (we assume in various places Windows developers have either set up their environment to be able to launch python scripts directly, or know to prepend "python" when doing so). https://codereview.chromium.org/2448133006/diff/180001/tools/traffic_annotati... File tools/traffic_annotation/README.txt (right): https://codereview.chromium.org/2448133006/diff/180001/tools/traffic_annotati... tools/traffic_annotation/README.txt:38: clang with keeprsp switch as follows: Explain why these steps are necessary and what this switch does. https://codereview.chromium.org/2448133006/diff/180001/tools/traffic_annotati... tools/traffic_annotation/README.txt:40: 2. add is_clang=true to the opened text file and save and close it. This will switch the developer's compiler to clang until they remember to switch back. You should explicitly note the need to switch back. https://codereview.chromium.org/2448133006/diff/180001/tools/traffic_annotati... tools/traffic_annotation/README.txt:45: used. Please address this.
rhalavati@chromium.org changed reviewers: - battre@chromium.org, pkasting@chromium.org
Comments addressed. https://codereview.chromium.org/2448133006/diff/180001/third_party/protobuf/B... File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2448133006/diff/180001/third_party/protobuf/B... third_party/protobuf/BUILD.gn:183: # Traffic_annotation is not part of the Chrome build. On 2016/11/23 17:01:52, Peter Kasting wrote: > Nit: Maybe clearer: "The traffic_annotation tool is not part of Chrome itself, > and needs to parse human-readable protobufs." Done.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
LGTM https://codereview.chromium.org/2448133006/diff/180001/tools/traffic_annotati... File tools/traffic_annotation/README.txt (right): https://codereview.chromium.org/2448133006/diff/180001/tools/traffic_annotati... tools/traffic_annotation/README.txt:38: clang with keeprsp switch as follows: On 2016/11/23 17:01:52, Peter Kasting wrote: > Explain why these steps are necessary and what this switch does. Still relevant. https://codereview.chromium.org/2448133006/diff/180001/tools/traffic_annotati... tools/traffic_annotation/README.txt:40: 2. add is_clang=true to the opened text file and save and close it. On 2016/11/23 17:01:52, Peter Kasting wrote: > This will switch the developer's compiler to clang until they remember to switch > back. You should explicitly note the need to switch back. Still relevant. https://codereview.chromium.org/2448133006/diff/270001/third_party/protobuf/p... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2448133006/diff/270001/third_party/protobuf/p... third_party/protobuf/proto_library.gni:64: # instead of protobuf_lite. Nit: Maybe clearer: Adds protobuf_full to the deps instead of protobuf_lite, for projects which need the full library. protobuf_full should not be used in Chromium itself, but may be needed by external tools.
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Dominic, Martin, The tool for extraction of network annotations and syntax checking is ready to land. I think it's good to land this one and then base the next version with partial annotations on this.
Hi Ramin! Conceptually looks good, I mostly commented on style. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... File tools/traffic_annotation/auditor/README.txt (right): https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.txt:1: This tool runs the clang tool for extraction of Network Traffic Annotations The official guidance is to use markdown (README.md). See here: https://chromium.googlesource.com/chromium/src/+/master/docs/documentation_gu... https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.txt:28: --summary_file=report.txt nit: Fits on the above line. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:5: // See README.txt for information and build instructions. Also here :) https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:22: Please use an anonymous namespace for helper classes. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:23: // Holds an instance of network traffic annotation nit: Period at the end of the sentence. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:24: struct Instance { This is very generic. How about AnnotationInstance? https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:27: traffic_annotation::NetworkTrafficAnnotation proto; Please add documentation for public attributes. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:30: enum { kOK, kDuplicate, kUndefined } state; https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md According to the style guide, kConstantNaming is only used for the "enum hack" to define an int constant; however, MACRO_STYLE is used for actual enumerations. States of an instance are an enumeration. I suggest one of the following: enum State { STATE_OK, STATE_DUPLICATE, STATE_UNDEFINED } or enum class State { OK, DUPLICATE, UNDEFINED } https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:33: // Runs clang tool, returns true if all OK. nit: 'if successful' ? https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:55: "python %s traffic_annotation_extractor %s %S --tool-args %s "); Is the capital S intentional? https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:61: int result2 = I would prefer more semantic names than |result1| and |result2|. Or actually we can just reuse |result|. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:78: // Reads an extrated annotation file. The first 6 lines of the file include: typo: extracted https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:87: std::vector<std::string>* header_lines, I assume that this is the first 6 lines and the below is the protobuf text. Please make this obvious in the method doc, e.g. "Reads an extracted annotation file and returns it in the output variables. The file starts with six |header_lines| with the following meaning:" https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:89: std::ifstream input_file(file_path.value()); Should we use base::File instead? https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:108: // Reads all extracted txt files form given input folder and populates instances typo: from https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:109: // and errors. What are errors in this case? https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:156: src->set_line(atoi(header_lines[2].c_str())); base::StringToInt https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:161: printf("%i annotation instance(s) read.\n", (int)instances->size()); You can use %ud instead of the cast. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:183: FILE* output_file = fopen(file_path.MaybeAsASCII().c_str(), "wt"); Ditto; should we use base::File? https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:199: fprintf(output_file, "------------------------------------------------\n"); How wide can the output above be? I think this separator should be longer, say, 80 chars. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:289: if (!RunClangTool(command_line.GetProgram().DirName().AppendASCII("../.."), Please add a comment to explain why two directories up.
rhalavati@chromium.org changed reviewers: + dcheng@chromium.org
msramek@: All comments addressed, please review. dcheng@: I've added a clang tool for extraction of network traffic annotations in tools/clang/traffic_annotation_extractor. Please review. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... File tools/traffic_annotation/auditor/README.txt (right): https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.txt:1: This tool runs the clang tool for extraction of Network Traffic Annotations On 2017/02/21 15:57:53, msramek (OOO until EOW) wrote: > The official guidance is to use markdown (README.md). See here: > https://chromium.googlesource.com/chromium/src/+/master/docs/documentation_gu... Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.txt:28: --summary_file=report.txt On 2017/02/21 15:57:53, msramek (OOO until EOW) wrote: > nit: Fits on the above line. Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > nit: 2017 Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:5: // See README.txt for information and build instructions. On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > Also here :) Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:22: On 2017/02/21 15:57:53, msramek (OOO until EOW) wrote: > Please use an anonymous namespace for helper classes. Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:23: // Holds an instance of network traffic annotation On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > nit: Period at the end of the sentence. Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:24: struct Instance { On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > This is very generic. How about AnnotationInstance? Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:27: traffic_annotation::NetworkTrafficAnnotation proto; On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > Please add documentation for public attributes. Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:30: enum { kOK, kDuplicate, kUndefined } state; On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > > > According to the style guide, kConstantNaming is only used for the "enum hack" > to define an int constant; however, MACRO_STYLE is used for actual enumerations. > > States of an instance are an enumeration. I suggest one of the following: > > enum State { > STATE_OK, > STATE_DUPLICATE, > STATE_UNDEFINED > } > > or > > enum class State { > OK, > DUPLICATE, > UNDEFINED > } Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:33: // Runs clang tool, returns true if all OK. On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > nit: 'if successful' ? Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:55: "python %s traffic_annotation_extractor %s %S --tool-args %s "); On 2017/02/21 15:57:53, msramek (OOO until EOW) wrote: > Is the capital S intentional? Yes, it's to convert |path| from ASCII to unicode. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:61: int result2 = On 2017/02/21 15:57:53, msramek (OOO until EOW) wrote: > I would prefer more semantic names than |result1| and |result2|. Or actually we > can just reuse |result|. Done, variable is temporary and used just to avoid computing twice for error reporting. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:78: // Reads an extrated annotation file. The first 6 lines of the file include: On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > typo: extracted Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:87: std::vector<std::string>* header_lines, On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > I assume that this is the first 6 lines and the below is the protobuf text. > Please make this obvious in the method doc, e.g. > > "Reads an extracted annotation file and returns it in the output variables. The > file starts with six |header_lines| with the following meaning:" Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:89: std::ifstream input_file(file_path.value()); On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > Should we use base::File instead? Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:108: // Reads all extracted txt files form given input folder and populates instances On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > typo: from Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:109: // and errors. On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > What are errors in this case? |errors| vector added, comment expanded. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:156: src->set_line(atoi(header_lines[2].c_str())); On 2017/02/21 15:57:53, msramek (OOO until EOW) wrote: > base::StringToInt Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:161: printf("%i annotation instance(s) read.\n", (int)instances->size()); On 2017/02/21 15:57:55, msramek (OOO until EOW) wrote: > You can use %ud instead of the cast. Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:183: FILE* output_file = fopen(file_path.MaybeAsASCII().c_str(), "wt"); On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > Ditto; should we use base::File? Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:199: fprintf(output_file, "------------------------------------------------\n"); On 2017/02/21 15:57:54, msramek (OOO until EOW) wrote: > How wide can the output above be? I think this separator should be longer, say, > 80 chars. Done. https://codereview.chromium.org/2448133006/diff/390001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:289: if (!RunClangTool(command_line.GetProgram().DirName().AppendASCII("../.."), On 2017/02/21 15:57:55, msramek (OOO until EOW) wrote: > Please add a comment to explain why two directories up. Done.
Description was changed from ========== Tool added to extract network traffic annotations. This is a clang tool that extracts all occurrences of network traffic annotattions and produces a report of annotated locations and text. BUG=656607 ========== to ========== Tool added to extract network traffic annotations. This is a clang tool that extracts all occurrences of network traffic annotattions and produces a report of annotated locations and text. BUG=656607 ==========
Thanks for the improvements. tools/traffic_annotation/auditor/ LGTM % a few more comments. https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:59: printf(" Executing generate_win_compdb returned code: %i.\n", result); Sorry, forgot about this. Can we use "base/logging.h" instead? stdout -> LOG(INFO) stderr -> LOG(ERROR) Here and everywhere. https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:89: // The file starts with six |header_lines| with the following meaning:: typo: two colons https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:327: // extracted by moving to directories up. typo: two
rhalavati@chromium.org changed reviewers: + asanka@chromium.org, pastarmovj@chromium.org
msramek@: Comments addressed. dcheng@: Please review tools/clang/traffic_annotation_extractor/* asanka@, pastarmovj@: Please review DEPS as owners of net and components/policy https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:59: printf(" Executing generate_win_compdb returned code: %i.\n", result); On 2017/02/27 20:00:51, msramek (recovering from OOO) wrote: > Sorry, forgot about this. Can we use "base/logging.h" instead? > > stdout -> LOG(INFO) > stderr -> LOG(ERROR) > > Here and everywhere. Done. https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:89: // The file starts with six |header_lines| with the following meaning:: On 2017/02/27 20:00:51, msramek (recovering from OOO) wrote: > typo: two colons Done. https://codereview.chromium.org/2448133006/diff/410001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:327: // extracted by moving to directories up. On 2017/02/27 20:00:51, msramek (recovering from OOO) wrote: > typo: two Done.
lgtm
I did not check until the very end... https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:3: tools/traffic_annotation/traffic_annotaion_auditor. Refer to it for help on how nit: puth path in `` https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:7: tools/clang/scripts/update.py --bootstrap --force-local-build nit ``` before line 7 and after line 9 https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:13: command prompt and run "depot_tools\win_toolchain\vs_fies\ vs_files? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:14: $long_autocompleted_hash\win_sdk\bin\setenv.cmd /x64" nit: command in ` ... ` https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:17: rewrite_to_chrome_style traffic_annotation_extractor again: ` ... ` https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:8: // (per instance) in the given output directory. you don't specify the parameters of this binary anywhere, do you? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:30: // a variable of this type as it's input parameter. Can you add examples of these two cases? I think that I don't fully understand them. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:42: // Name of the function including this instance. What does this mean? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:42: // Name of the function including this instance. Nit (also below): newline before // https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:46: // a function or net_annotation when it's a variable. I don't understand this either. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:50: // Annotation content. // Annotation content: These are the parameters of a call to Define...; the unique_id is an identifier for the annotation that has to be unique across the entire code base. The |text| stores a RAW string with the annotation that should be extracted. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:60: std::string error; extra linebreaks (see above) https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:63: // Flag stating that parameter is directly passed to annotate function here what is "parameter" here? I think this is an unresolved reference. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:67: // function. Please give an example as well. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:83: bool net_match(const std::string& token, const std::string& name) { I think that the style guide requires CamelCase here: "IsNetMatch" But I wonder whether we can find a better function name or call style. Would it make sense to have a function "StripNetNamespace(const std::string& token)" ? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:88: // string const, etc. string literal? What's a string const? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:109: // Raw string? Can you explain the following branches better? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:113: if (auto* c1 = clang::dyn_cast<clang::StringLiteral>(token)) why "c"? if (auto* literal = ...) ? similarly below. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:149: std::string GetCoveringFunction(const clang::Stmt* token, This overloading is discouraged: https://google.github.io/styleguide/cppguide.html#Function_Overloading https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:153: // TODO: What exactly != 1 parent mean? Can you resolve this TODO? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:168: return f->getQualifiedNameAsString(); This path is not documented in the function comment. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:182: // Finds file name and line number of the given token. ... and writes it into |location|. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:185: const MatchFinder::MatchResult& result, Do you want to pass the SourceManager instead of the result here? I think that this makes more sense for a reader of the function. I did not understand why you pass a result. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:224: // Mark it as transitive parameter if it doesn't have initialization but what is "it"? Can you give it a name? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:231: GetAnnotationText(call_expr, *result.SourceManager, &instance); I think that here is some inconsistency: GetLocation passes a result, but GetAnnotationText a SourceManager. WDYT about AddVariable and AddFunction only accepting a var_decl/call_expr and a SourceManager? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:331: std::string output_dir(argv[4]); I think this parameter is not required by the readme file. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:333: argc = 4; Wouldn't it be cleaner introduce a new parameter --out_dir and let the CommonOptionsParser take care of parsing this? See https://clang.llvm.org/doxygen/CommonOptionsParser_8cpp_source.html https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:336: clang::tooling::CommonOptionsParser options(argc, argv, category); CommonParserOptions supports a help messages https://clang.llvm.org/doxygen/classclang_1_1tooling_1_1CommonOptionsParser.html https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:347: // llvm::outs() << "==== END EDITS ====\n"; Can this be deleted? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:351: for (auto& c : collector.calls) { I think that for (NetworkAnnotationInstance& call : collector.calls) would be more readable. https://google.github.io/styleguide/cppguide.html#auto https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2448133006/diff/450001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/450001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:51: result = system( I think you should use base/process/launch.h
Sorry, I've been a bit swamped. Some high-level comments are: - add some tests (see examples in rewrite_to_chrome_style) - prefer LLVMisms where it makes sense, e.g. use llvm::StringRef rather than converting to std::string when not needed, or using LLVMisms for file I/O: https://cs.chromium.org/chromium/src/tools/clang/rewrite_to_chrome_style/Rewr... https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:42: Location() : line_number(-1) {} Consider using in-class member initializers. The constructor can be removed, and line 44 changed to: int line_number = -1; https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:78: // When this structure is refering to a function with a parameter of type Nit: refering => referring https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:122: clang::LangOptions lopt; Nit: use the LangOptions from ASTContext. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:169: GetStmtText(call_expr->getArgs()[0], source_manager); It'd be ideal to take advantage of matcher binding to bind directly to the string literals using the id() matcher. That would probably allow this to be simplified to just call StringLiteral::getString(). https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:324: instance.error = "Unknwon parameter type."; Nit: unknown https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:346: varDecl(anyOf(hasType(asString("NetworkTrafficAnnotationTag")), Can you help me understand why we need both? Shouldn't this always be in the net namespace? https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:375: // Keep to consumed parameter from being passed to clang parser. Maybe just make this a proper flag? https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:416: std::ofstream output_file(file_path); How are reads/writes to this file synchronized? If there are parallel invocations, I think the file context will get clobbered.
Daniel, Dominic, Clang tool code is refactored and simplified by removing an unnecessary feature. All former comments are applied, but some of their references are now removed from the code. In all cases that there was a clarification question, the answer is added to code comments. Please review. About tests, I added 2 sample input files and their expected output files. Is there any method to do an automated test for clang tools, similar to presubmit check, or they can stay as they are? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:3: tools/traffic_annotation/traffic_annotaion_auditor. Refer to it for help on how On 2017/02/28 18:25:11, battre wrote: > nit: puth path in `` Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:7: tools/clang/scripts/update.py --bootstrap --force-local-build On 2017/02/28 18:25:11, battre wrote: > nit ``` before line 7 and after line 9 Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:13: command prompt and run "depot_tools\win_toolchain\vs_fies\ On 2017/02/28 18:25:11, battre wrote: > vs_files? Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:14: $long_autocompleted_hash\win_sdk\bin\setenv.cmd /x64" On 2017/02/28 18:25:11, battre wrote: > nit: command in ` ... ` Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:17: rewrite_to_chrome_style traffic_annotation_extractor On 2017/02/28 18:25:11, battre wrote: > again: ` ... ` Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:8: // (per instance) in the given output directory. On 2017/02/28 18:25:11, battre wrote: > you don't specify the parameters of this binary anywhere, do you? This binary is not designed for direct calling and is run using traffic_annotation_auditor. I added the parameters to README.md. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:30: // a variable of this type as it's input parameter. On 2017/02/28 18:25:12, battre wrote: > Can you add examples of these two cases? I think that I don't fully understand > them. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:42: // Name of the function including this instance. On 2017/02/28 18:25:12, battre wrote: > Nit (also below): newline before // Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:42: // Name of the function including this instance. On 2017/02/28 18:25:11, battre wrote: > What does this mean? Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:46: // a function or net_annotation when it's a variable. On 2017/02/28 18:25:11, battre wrote: > I don't understand this either. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:50: // Annotation content. On 2017/02/28 18:25:12, battre wrote: > // Annotation content: These are the parameters of a call to Define...; the > unique_id is an identifier for the annotation that has to be unique across the > entire code base. The |text| stores a RAW string with the annotation that should > be extracted. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:60: std::string error; On 2017/02/28 18:25:11, battre wrote: > extra linebreaks (see above) Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:63: // Flag stating that parameter is directly passed to annotate function here On 2017/02/28 18:25:12, battre wrote: > what is "parameter" here? I think this is an unresolved reference. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:67: // function. On 2017/02/28 18:25:12, battre wrote: > Please give an example as well. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:83: bool net_match(const std::string& token, const std::string& name) { On 2017/02/28 18:25:12, battre wrote: > I think that the style guide requires CamelCase here: "IsNetMatch" > > But I wonder whether we can find a better function name or call style. Would it > make sense to have a function "StripNetNamespace(const std::string& token)" ? How about "StripNetNamespaceMatch"? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:88: // string const, etc. On 2017/02/28 18:25:12, battre wrote: > string literal? What's a string const? Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:109: // Raw string? On 2017/02/28 18:25:12, battre wrote: > Can you explain the following branches better? Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:113: if (auto* c1 = clang::dyn_cast<clang::StringLiteral>(token)) On 2017/02/28 18:25:12, battre wrote: > why "c"? if (auto* literal = ...) ? > > similarly below. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:149: std::string GetCoveringFunction(const clang::Stmt* token, On 2017/02/28 18:25:12, battre wrote: > This overloading is discouraged: > https://google.github.io/styleguide/cppguide.html#Function_Overloading Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:153: // TODO: What exactly != 1 parent mean? On 2017/02/28 18:25:11, battre wrote: > Can you resolve this TODO? I haven't found a clue yet. I am hoping that clang reviewer will tell me how can one object have two parents here. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:168: return f->getQualifiedNameAsString(); On 2017/02/28 18:25:12, battre wrote: > This path is not documented in the function comment. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:182: // Finds file name and line number of the given token. On 2017/02/28 18:25:11, battre wrote: > ... and writes it into |location|. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:185: const MatchFinder::MatchResult& result, On 2017/02/28 18:25:12, battre wrote: > Do you want to pass the SourceManager instead of the result here? I think that > this makes more sense for a reader of the function. I did not understand why you > pass a result. Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:224: // Mark it as transitive parameter if it doesn't have initialization but On 2017/02/28 18:25:11, battre wrote: > what is "it"? Can you give it a name? Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:231: GetAnnotationText(call_expr, *result.SourceManager, &instance); On 2017/02/28 18:25:12, battre wrote: > I think that here is some inconsistency: GetLocation passes a result, but > GetAnnotationText a SourceManager. WDYT about AddVariable and AddFunction only > accepting a var_decl/call_expr and a SourceManager? AddVariable can be changed, but AddFunction requires result to pass its context to GetStatementCoveringFunction. Do you suggest I change one and keep the other as it is? Or add two paremeters source_manager and context to AddFunction? https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:331: std::string output_dir(argv[4]); On 2017/02/28 18:25:11, battre wrote: > I think this parameter is not required by the readme file. I don't understand, elaborate please. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:333: argc = 4; On 2017/02/28 18:25:12, battre wrote: > Wouldn't it be cleaner introduce a new parameter --out_dir and let the > CommonOptionsParser take care of parsing this? > > See https://clang.llvm.org/doxygen/CommonOptionsParser_8cpp_source.html Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:336: clang::tooling::CommonOptionsParser options(argc, argv, category); On 2017/02/28 18:25:12, battre wrote: > CommonParserOptions supports a help messages > https://clang.llvm.org/doxygen/classclang_1_1tooling_1_1CommonOptionsParser.html Done. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:347: // llvm::outs() << "==== END EDITS ====\n"; On 2017/02/28 18:25:12, battre wrote: > Can this be deleted? It was a requirement in previous version of clang run tools script, but is removed now. https://codereview.chromium.org/2448133006/diff/450001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:351: for (auto& c : collector.calls) { On 2017/02/28 18:25:11, battre wrote: > I think that for (NetworkAnnotationInstance& call : collector.calls) would be > more readable. > > https://google.github.io/styleguide/cppguide.html#auto > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Done. https://codereview.chromium.org/2448133006/diff/450001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/450001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:51: result = system( On 2017/02/28 18:25:12, battre wrote: > I think you should use base/process/launch.h Done. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:42: Location() : line_number(-1) {} On 2017/03/02 07:57:41, dcheng wrote: > Consider using in-class member initializers. The constructor can be removed, and > line 44 changed to: > > int line_number = -1; Done. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:78: // When this structure is refering to a function with a parameter of type On 2017/03/02 07:57:41, dcheng wrote: > Nit: refering => referring Done. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:122: clang::LangOptions lopt; On 2017/03/02 07:57:41, dcheng wrote: > Nit: use the LangOptions from ASTContext. Done. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:169: GetStmtText(call_expr->getArgs()[0], source_manager); On 2017/03/02 07:57:41, dcheng wrote: > It'd be ideal to take advantage of matcher binding to bind directly to the > string literals using the id() matcher. That would probably allow this to be > simplified to just call StringLiteral::getString(). Done. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:324: instance.error = "Unknwon parameter type."; On 2017/03/02 07:57:41, dcheng wrote: > Nit: unknown Done. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:346: varDecl(anyOf(hasType(asString("NetworkTrafficAnnotationTag")), On 2017/03/02 07:57:41, dcheng wrote: > Can you help me understand why we need both? Shouldn't this always be in the net > namespace? If the code has the line "using namespace net;", then this can be called without namespace. Should we assume it's not and remove the first one? https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:375: // Keep to consumed parameter from being passed to clang parser. On 2017/03/02 07:57:41, dcheng wrote: > Maybe just make this a proper flag? Done. https://codereview.chromium.org/2448133006/diff/470001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:416: std::ofstream output_file(file_path); On 2017/03/02 07:57:41, dcheng wrote: > How are reads/writes to this file synchronized? If there are parallel > invocations, I think the file context will get clobbered. Comment updated in refactored source: "For each call to "DefineNetworkTrafficAnnotation", write annotation text and relevant meta data into a separate file. The filename is uniquely generated using the file path and filename of the code including the call and it's line number." These files are aggre using traffic_annotation_auditor.cc
https://codereview.chromium.org/2448133006/diff/610001/third_party/protobuf/B... File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2448133006/diff/610001/third_party/protobuf/B... third_party/protobuf/BUILD.gn:232: "src/google/protobuf/any.cc", I think that this indentation with tabs is wrong and should be reverted. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:3: `tools/traffic_annotation/traffic_annotaion_auditor`. Refer to it for help on `tools/clang/...` but please check the entire path. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:6: # Build on Linux ## https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:8: --without-android --extra-tools traffic_annotation_extractor` Are you sure that ` ` can contain wrapped lines? Do you need ```...```? add a \ at the end of the line wrapping for bash compliance? https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:10: # Build on Window ## https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:18: Run `traffic_annotation_extractor -help` for parameters help. isn't this --help? https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:19: Extracts network traffic annotations from given file paths based on build The executable extracts ... https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:20: parameters in build path, and writes them as separate files in output into an output directory https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/tests/inputs/autofill_download_manager.cc (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/tests/inputs/autofill_download_manager.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. I would suggest to reduce this file significantly as it only needs to serve as an input for a unittest. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/tests/inputs/payments_client.cc (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/tests/inputs/payments_client.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Same here. I think that we want unittests, not integration tests. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:29: // An instance of call to net::DefineNetworkTrafficAnnotation function. An instance of *a* to *the* ... function. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:36: // Name of the function including this instance. E.g., in the following I don't understand "Name of the function including this instance" "Name of the function calling net::DefineNetworkTrafficAnnotation"? Are namespaces ignored or considered? https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:43: // Annotation content. These are the parameters of a call to of the call https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:46: // stores a Raw string with the annotation that should be extracted. raw https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:58: // Returns the function that includes the given token. For example, if the token s/includes/contains/ https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:63: // Returns the function that includes the given token. For example, if the token s/includes/contains/ https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:71: // If parent is found, extract its name recursively. Can you explain this to me? In which case would we have a parent? And what would be returned in this case? https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:82: // Returns the function that includes the given token. For example, if the token contains https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:106: // are defined in RunMatchers function and when a pattern is found there, "function. When a" ... https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... File tools/traffic_annotation/auditor/README.md (right): https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:6: ninja -C [build directory] traffic_annotation_auditor `...` https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:9: traffic_annotation_auditor [OPTION]... [path_filter]... `...` https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:13: Options: did you check how the following lines are rendered by markdown? should all of this go into ``` ```? https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:34: 1. gn args [build_dir, e.g. out\Debug] here and in the following ` ` https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:68: src_dir.AppendASCII("tools/clang/scripts/run_tool.py").MaybeAsASCII())); Do you need to go through ASCII here and in the following? Doesn't that break if src_dir contains non-ascii characters? Can't you work with .value()? https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:189: if (match != unique_ids.end()) please use {} if lines are wrapped also ein the else branch. https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:302: "extract annotations, or specify extracted files's input " You must either specify the build directory to run clang tool and extract annotations, or specify specify the input directory where extracted annotation files already exist.
All comments addressed, please review. https://codereview.chromium.org/2448133006/diff/610001/third_party/protobuf/B... File third_party/protobuf/BUILD.gn (right): https://codereview.chromium.org/2448133006/diff/610001/third_party/protobuf/B... third_party/protobuf/BUILD.gn:232: "src/google/protobuf/any.cc", On 2017/04/07 08:42:27, battre wrote: > I think that this indentation with tabs is wrong and should be reverted. Done. They were changed by git cl format. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:3: `tools/traffic_annotation/traffic_annotaion_auditor`. Refer to it for help on On 2017/04/07 08:42:27, battre wrote: > `tools/clang/...` but please check the entire path. Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:6: # Build on Linux On 2017/04/07 08:42:27, battre wrote: > ## Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:8: --without-android --extra-tools traffic_annotation_extractor` On 2017/04/07 08:42:27, battre wrote: > Are you sure that ` ` can contain wrapped lines? Do you need ```...```? > > add a \ at the end of the line wrapping for bash compliance? Markdown viewer shows it correctly. Should it be changed? https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:10: # Build on Window On 2017/04/07 08:42:27, battre wrote: > ## Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:18: Run `traffic_annotation_extractor -help` for parameters help. On 2017/04/07 08:42:27, battre wrote: > isn't this --help? Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:19: Extracts network traffic annotations from given file paths based on build On 2017/04/07 08:42:27, battre wrote: > The executable extracts ... Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/README.md:20: parameters in build path, and writes them as separate files in output On 2017/04/07 08:42:27, battre wrote: > into an output directory Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/tests/inputs/autofill_download_manager.cc (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/tests/inputs/autofill_download_manager.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/04/07 08:42:27, battre wrote: > I would suggest to reduce this file significantly as it only needs to serve as > an input for a unittest. I am not sure if this is the correct way to add tests here, because the samples require to be part of the build process to be compilable with clang tool. Let's wait for Daniel's feedback on how should clang tests be added and then I will update all of the files in tests directory. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/tests/inputs/payments_client.cc (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/tests/inputs/payments_client.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/04/07 08:42:27, battre wrote: > Same here. I think that we want unittests, not integration tests. Ditto. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:29: // An instance of call to net::DefineNetworkTrafficAnnotation function. On 2017/04/07 08:42:28, battre wrote: > An instance of *a* to *the* ... function. Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:36: // Name of the function including this instance. E.g., in the following On 2017/04/07 08:42:27, battre wrote: > I don't understand "Name of the function including this instance" > > "Name of the function calling net::DefineNetworkTrafficAnnotation"? > > Are namespaces ignored or considered? Done. Namespaces are ignored and if DefineNetworkTrafficAnnotation function is called outside any function, "Unknown" value will be returned as function name (because it might also happen based on other possible errors that I don't know about, but if it is important, I can try to distinguish between them). https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:43: // Annotation content. These are the parameters of a call to On 2017/04/07 08:42:27, battre wrote: > of the call Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:46: // stores a Raw string with the annotation that should be extracted. On 2017/04/07 08:42:28, battre wrote: > raw Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:58: // Returns the function that includes the given token. For example, if the token On 2017/04/07 08:42:28, battre wrote: > s/includes/contains/ Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:63: // Returns the function that includes the given token. For example, if the token On 2017/04/07 08:42:27, battre wrote: > s/includes/contains/ Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:71: // If parent is found, extract its name recursively. On 2017/04/07 08:42:28, battre wrote: > Can you explain this to me? In which case would we have a parent? And what would > be returned in this case? Frankly I don't know and I couldn't find any documentation on this. I had assertions in code to find instances with non-one parent to find out what it means, but it has always returned just 1 parent. Daniel, can you help? https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:82: // Returns the function that includes the given token. For example, if the token On 2017/04/07 08:42:27, battre wrote: > contains Done. https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:106: // are defined in RunMatchers function and when a pattern is found there, On 2017/04/07 08:42:28, battre wrote: > "function. When a" ... Done. https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... File tools/traffic_annotation/auditor/README.md (right): https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:6: ninja -C [build directory] traffic_annotation_auditor On 2017/04/07 08:42:28, battre wrote: > `...` https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:9: traffic_annotation_auditor [OPTION]... [path_filter]... On 2017/04/07 08:42:28, battre wrote: > `...` Done. https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:13: Options: On 2017/04/07 08:42:28, battre wrote: > did you check how the following lines are rendered by markdown? > > should all of this go into ``` ```? Replaced them with a reference to --help as it was redundant. https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:34: 1. gn args [build_dir, e.g. out\Debug] On 2017/04/07 08:42:28, battre wrote: > here and in the following ` ` Done. https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:68: src_dir.AppendASCII("tools/clang/scripts/run_tool.py").MaybeAsASCII())); On 2017/04/07 08:42:28, battre wrote: > Do you need to go through ASCII here and in the following? Doesn't that break if > src_dir contains non-ascii characters? > > Can't you work with .value()? I couldn't make it work any simpler way. I will discuss it with Windows developers group to see if I can force windows compile to be ascii similar to linux compile, this way I will get read of all #ifdef _WIN32 parts. https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:189: if (match != unique_ids.end()) On 2017/04/07 08:42:28, battre wrote: > please use {} if lines are wrapped also ein the else branch. Done. https://codereview.chromium.org/2448133006/diff/610001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:302: "extract annotations, or specify extracted files's input " On 2017/04/07 08:42:28, battre wrote: > You must either specify the build directory to run clang tool and extract > annotations, or specify specify the input directory where extracted annotation > files already exist. Done.
https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/tests/inputs/autofill_download_manager.cc (right): https://codereview.chromium.org/2448133006/diff/610001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/tests/inputs/autofill_download_manager.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/04/07 11:33:32, Ramin Halavati wrote: > On 2017/04/07 08:42:27, battre wrote: > > I would suggest to reduce this file significantly as it only needs to serve as > > an input for a unittest. > > I am not sure if this is the correct way to add tests here, because the samples > require to be part of the build process to be compilable with clang tool. Let's > wait for Daniel's feedback on how should clang tests be added and then I will > update all of the files in tests directory. SGTM. Keep in mind that the current style of testing means that some poor owner of components/autofill/core/browser/autofill_download_manager.h needs to change these files if they add a new function. I think that is not a good idea. https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:139: // If DefineNetworkTrafficAnnotation is used in form of a macro, empty an empty https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:140: // file_path is returned. Traversing to its parrent node in this case parent https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:141: // will result in correct value. in the correct https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... File tools/traffic_annotation/auditor/README.md (right): https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:6: `ninja -C [build directory] trafmarkfic_annotation_auditor` trafmarkfic? https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:46: base::CommandLine::StringVector argv; Would this work (for Windows+POSIX)? I haven't tried to compile it. Please test whether it works if output_dir contains spaces. base::CommandLine cl( FilePath(FILE_PATH_LITERAL("tools")) .Append(FILE_PATH_LITERAL("clang")) .Append(FILE_PATH_LITERAL("cscripts")) .Append(FILE_PATH_LITERAL("generate_win_compdb.py"))); cl.PrependWrapper(FILE_PATH_LITERAL("python")); cl.AppendSwitch("generate-compdb"); base::CommandLine tool_args_cmd(NO_PROGRAMM); tool_args_cmd.AppendArgPath("output-directory", output_dir); cl.AppendSwitchNative("tool-args", tools_arg_cmd.GetArgs()[1])); cl.AppendArg("traffic_annotation_extractor"); cl.AppendArgPath(build_dir); cl.AppendArgPath(path);
Comments addressed, please review. https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:139: // If DefineNetworkTrafficAnnotation is used in form of a macro, empty On 2017/04/10 10:58:31, battre wrote: > an empty Done. https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:140: // file_path is returned. Traversing to its parrent node in this case On 2017/04/10 10:58:31, battre wrote: > parent Done. https://codereview.chromium.org/2448133006/diff/650001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:141: // will result in correct value. On 2017/04/10 10:58:31, battre wrote: > in the correct Done. https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... File tools/traffic_annotation/auditor/README.md (right): https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... tools/traffic_annotation/auditor/README.md:6: `ninja -C [build directory] trafmarkfic_annotation_auditor` On 2017/04/10 10:58:31, battre wrote: > trafmarkfic? Done! https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/650001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:46: base::CommandLine::StringVector argv; On 2017/04/10 10:58:31, battre wrote: > Would this work (for Windows+POSIX)? I haven't tried to compile it. Please test > whether it works if output_dir contains spaces. > > base::CommandLine cl( > FilePath(FILE_PATH_LITERAL("tools")) > .Append(FILE_PATH_LITERAL("clang")) > .Append(FILE_PATH_LITERAL("cscripts")) > .Append(FILE_PATH_LITERAL("generate_win_compdb.py"))); > cl.PrependWrapper(FILE_PATH_LITERAL("python")); > > cl.AppendSwitch("generate-compdb"); > > base::CommandLine tool_args_cmd(NO_PROGRAMM); > tool_args_cmd.AppendArgPath("output-directory", output_dir); > cl.AppendSwitchNative("tool-args", tools_arg_cmd.GetArgs()[1])); > > cl.AppendArg("traffic_annotation_extractor"); > cl.AppendArgPath(build_dir); > cl.AppendArgPath(path); Done.
https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:60: llvm::StringRef GetDeclarationCoveringFunction(const clang::Decl* token, It's usually nicer to express things as matchers when possible. So we could do: auto is_define_network_traffic_annotation = hasDeclaration(...); match_finder.addMatcher(callExpr( is_define_network_traffic_annotation, hasAncestor(functionDecl().bind("functionContext")), &in_function_callback); match_finder.addMatcher(callExpr( is_define_network_traffic_annotation, unless(hasAncestor(functionDecl())), ¬_in_function_callback); WDYT? We could actually just use the same callback for both, and gate it on whether or not functionContext is bound. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:102: return llvm::StringRef("Unknown"); Btw, no need to explicit construct StringRef here. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:125: if (call_expr && unique_id && annotation_text) { Let's assert since we should expect all these to be non-null. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:142: if (!instance.location.file_path.length()) { Can we do something with getImmediateMacroCallerLoc() to jump out of scratch space? For example, see https://cs.chromium.org/chromium/src/tools/clang/rewrite_to_chrome_style/Rewr... https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:160: instance.location.file_path.substr(0, 3) == "../") { I would be cautious of doing this, as Windows will sometimes have \ in the name. One easy hack to work around this is to just replace all \\ in the name with / -- we can't allow filenames like that in the repo, since it won't work on Windows. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:219: // For each call to "DefineNetworkTrafficAnnotation", write annotation text Btw, I'm curious if we still need to do this: for the Blink rename, we split the tool up into several stages. Before, run_tool.py would do everything, including applying edits. Now run_tool.py prints the stdout of tools to stdout, so the previous pipeline now looks like: run_tool.py > output cat output extract_edits.py| apply_edits.py I think for network annotations, we can just substitute something else other than extract_edits.py | apply_edits.py. That should make the logic simpler overall for traffic_annotation_auditor.cc https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:222: // and it's line number. Nit: it's -> its
Thanks for the wonderful suggestions Daniel. I have updated the clang too, please review. I will update the other cc after this. Do you have any comments about the tests? https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:60: llvm::StringRef GetDeclarationCoveringFunction(const clang::Decl* token, On 2017/04/11 07:26:42, dcheng wrote: > It's usually nicer to express things as matchers when possible. > > So we could do: > > auto is_define_network_traffic_annotation = hasDeclaration(...); > match_finder.addMatcher(callExpr( > is_define_network_traffic_annotation, > hasAncestor(functionDecl().bind("functionContext")), > &in_function_callback); > match_finder.addMatcher(callExpr( > is_define_network_traffic_annotation, > unless(hasAncestor(functionDecl())), > ¬_in_function_callback); > > WDYT? We could actually just use the same callback for both, > and gate it on whether or not functionContext is bound. WOW! That was wonderful. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:102: return llvm::StringRef("Unknown"); On 2017/04/11 07:26:42, dcheng wrote: > Btw, no need to explicit construct StringRef here. Done. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:125: if (call_expr && unique_id && annotation_text) { On 2017/04/11 07:26:42, dcheng wrote: > Let's assert since we should expect all these to be non-null. Done. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:142: if (!instance.location.file_path.length()) { On 2017/04/11 07:26:42, dcheng wrote: > Can we do something with getImmediateMacroCallerLoc() to jump out of scratch > space? For example, see > https://cs.chromium.org/chromium/src/tools/clang/rewrite_to_chrome_style/Rewr... Done. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:160: instance.location.file_path.substr(0, 3) == "../") { On 2017/04/11 07:26:42, dcheng wrote: > I would be cautious of doing this, as Windows will sometimes have \ in the name. > > One easy hack to work around this is to just replace all \\ in the name with / > -- we can't allow filenames like that in the repo, since it won't work on > Windows. I did the replacement, but I am not sure if I got the last sentence correctly. The reason I am doing this the file paths are passed to us starting with several "../"s and I want to remove it from temporary file names. https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:219: // For each call to "DefineNetworkTrafficAnnotation", write annotation text On 2017/04/11 07:26:42, dcheng wrote: > Btw, I'm curious if we still need to do this: for the Blink rename, we split the > tool up into several stages. Before, run_tool.py would do everything, including > applying edits. > > Now run_tool.py prints the stdout of tools to stdout, so the previous pipeline > now looks like: > > run_tool.py > output > cat output extract_edits.py| apply_edits.py > > I think for network annotations, we can just substitute something else other > than extract_edits.py | apply_edits.py. That should make the logic simpler > overall for traffic_annotation_auditor.cc Thanks, Done! https://codereview.chromium.org/2448133006/diff/670001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:222: // and it's line number. On 2017/04/11 07:26:42, dcheng wrote: > Nit: it's -> its Done.
https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:12: #include <fstream> Nit: I think this is unused now (and maybe stdio.h too?) https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:40: // If no function is found, 'Global namepace' will be returned. Nit: "Global Namespace" for consistency with what's actually returned https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:57: using Collector = std::vector<NetworkAnnotationInstance>; Nit: #include <vector> https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:101: if (!instance.location.file_path.length()) { Nit: .empty() Or maybe even just check source_location.isMacroID(), since an empty file_path is kind of an implementation detail of clang. https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:129: NetworkAnnotationTagCallback call_back(collector); nit: callback (for consistency in word breaking with Callback) https://codereview.chromium.org/2448133006/diff/690001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/690001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:35: enum State { STATE_OK, STATE_DUPLICATE, STATE_UNDEFINED } state; It's nice to use scoped enum where possible: enum class State { kOk, kDuplicate, kUndefined } state; https://codereview.chromium.org/2448133006/diff/690001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:107: void ReadExtractedFiles(const base::FilePath& folder_name, Probably this (and other similar) logic isn't needed anymore.
Daniel, Thank you very much, Clang tool updated, please review. Do you have any suggestions or comments about clang tool tests? Are they OK the way they are? P.S. I am replacing traffic_annotation_auditor.cc with traffic_annotation_auditor.py, but it is not completed yet. When done, I will remove the cc and unnecessary build commands. https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:12: #include <fstream> On 2017/04/12 07:07:05, dcheng wrote: > Nit: I think this is unused now (and maybe stdio.h too?) Done. https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:40: // If no function is found, 'Global namepace' will be returned. On 2017/04/12 07:07:05, dcheng wrote: > Nit: "Global Namespace" > > for consistency with what's actually returned Done. https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:57: using Collector = std::vector<NetworkAnnotationInstance>; On 2017/04/12 07:07:05, dcheng wrote: > Nit: #include <vector> Done. https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:101: if (!instance.location.file_path.length()) { On 2017/04/12 07:07:05, dcheng wrote: > Nit: .empty() > > Or maybe even just check source_location.isMacroID(), since an empty file_path > is kind of an implementation detail of clang. Done. https://codereview.chromium.org/2448133006/diff/690001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:129: NetworkAnnotationTagCallback call_back(collector); On 2017/04/12 07:07:05, dcheng wrote: > nit: callback (for consistency in word breaking with Callback) Done. https://codereview.chromium.org/2448133006/diff/690001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.cc (right): https://codereview.chromium.org/2448133006/diff/690001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:35: enum State { STATE_OK, STATE_DUPLICATE, STATE_UNDEFINED } state; On 2017/04/12 07:07:05, dcheng wrote: > It's nice to use scoped enum where possible: > > enum class State { kOk, kDuplicate, kUndefined } state; > Acknowledged. https://codereview.chromium.org/2448133006/diff/690001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.cc:107: void ReadExtractedFiles(const base::FilePath& folder_name, On 2017/04/12 07:07:05, dcheng wrote: > Probably this (and other similar) logic isn't needed anymore. I am trying to remove this file and replace it with a simpler python script.
On 2017/04/12 13:24:59, Ramin Halavati wrote: > Daniel, > > Thank you very much, Clang tool updated, please review. > > Do you have any suggestions or comments about clang tool tests? Are they OK the > way they are? Re: testing. I would suggest checking in a simple script that runs the tool with a preset compile DB and preset inputs, and verifies that the output matches some golden files. This is the approach used by plugin tests in blink_gc_plugin and plugins. The tool code LGTM -- let me know if you'd like me to look at anything else. https://codereview.chromium.org/2448133006/diff/710001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/710001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:39: // If no function is found, 'Global Namepace' will be returned. Nit: missing s in Name_s_pace =) https://codereview.chromium.org/2448133006/diff/710001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:164: for (NetworkAnnotationInstance& call : collector) { nit: const as well (it's also acceptable to just write const auto& here, but that's up to you)
Martin, Dominic, I replaced traffic_annotation_auditor.cc with .py script. I also changed test set for clang tool. I think it's better if we just have a manual test set for it now and add a bug to add automatic tests later. Please review. https://codereview.chromium.org/2448133006/diff/710001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2448133006/diff/710001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:39: // If no function is found, 'Global Namepace' will be returned. On 2017/04/12 22:03:56, dcheng wrote: > Nit: missing s in Name_s_pace =) Done. https://codereview.chromium.org/2448133006/diff/710001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:164: for (NetworkAnnotationInstance& call : collector) { On 2017/04/12 22:03:56, dcheng wrote: > nit: const as well > > (it's also acceptable to just write const auto& here, but that's up to you) Done.
Patchset #43 (id:830001) has been deleted
Getting there... I still don't like the testing. The rest are just nits. https://codereview.chromium.org/2448133006/diff/870001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/tests/README.md (right): https://codereview.chromium.org/2448133006/diff/870001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/tests/README.md:1: # Manual tests for Traffic Annotation Extractor This testing makes no sense to me. What happens if those two files that are completely out of control of code change? Here is my proposal: 1) Create a file tools/clang/traffic_annotation_extractor/tests/test.cc with all kinds of annotations you can think of. 2) Create a file tools/clang/traffic_annotation_extractor/tests/expected_outputs/test.txt 3) Create a file tools/clang/traffic_annotation_extractor/unittests.py that runs the extraction and verifies that the output matches the expected output. Does that work? https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.py (right): https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:32: raw_annotaions: str Output of clang tool, extracted content and meta data of typo: raw_annotations Should this be "Output of clang tool (extracted content and metadata of annotations)"? https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:56: raw_annotations: str Serialization of annotations and meta data. Each nit: metadata https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:81: "Error at line %i, expected starting new annotaion." % current) +2 indent (also below) https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:139: annotaitions ExtractedNetworkTrafficAnnotation A protobuf including all typo: annotaitions https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:158: 'annotations will be stored.') will be stored into. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:160: help='Optional Path to the file that temporary extracted ' typo: path https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:185: return return 1 https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:203: return return 1 https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:207: return 0
Comments addressed, please review. https://codereview.chromium.org/2448133006/diff/870001/tools/clang/traffic_an... File tools/clang/traffic_annotation_extractor/tests/README.md (right): https://codereview.chromium.org/2448133006/diff/870001/tools/clang/traffic_an... tools/clang/traffic_annotation_extractor/tests/README.md:1: # Manual tests for Traffic Annotation Extractor On 2017/04/20 13:41:03, battre wrote: > This testing makes no sense to me. What happens if those two files that are > completely out of control of code change? > > Here is my proposal: > > 1) Create a file tools/clang/traffic_annotation_extractor/tests/test.cc with all > kinds of annotations you can think of. > 2) Create a file > tools/clang/traffic_annotation_extractor/tests/expected_outputs/test.txt > 3) Create a file tools/clang/traffic_annotation_extractor/unittests.py that runs > the extraction and verifies that the output matches the expected output. > > Does that work? Acknowledged. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... File tools/traffic_annotation/auditor/traffic_annotation_auditor.py (right): https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:32: raw_annotaions: str Output of clang tool, extracted content and meta data of On 2017/04/20 13:41:03, battre wrote: > typo: raw_annotations > > Should this be "Output of clang tool (extracted content and metadata of > annotations)"? Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:56: raw_annotations: str Serialization of annotations and meta data. Each On 2017/04/20 13:41:04, battre wrote: > nit: metadata Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:81: "Error at line %i, expected starting new annotaion." % current) On 2017/04/20 13:41:04, battre wrote: > +2 indent (also below) Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:139: annotaitions ExtractedNetworkTrafficAnnotation A protobuf including all On 2017/04/20 13:41:03, battre wrote: > typo: annotaitions Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:158: 'annotations will be stored.') On 2017/04/20 13:41:04, battre wrote: > will be stored into. Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:160: help='Optional Path to the file that temporary extracted ' On 2017/04/20 13:41:04, battre wrote: > typo: path Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:185: return On 2017/04/20 13:41:04, battre wrote: > return 1 Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:203: return On 2017/04/20 13:41:04, battre wrote: > return 1 Done. https://codereview.chromium.org/2448133006/diff/870001/tools/traffic_annotati... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:207: On 2017/04/20 13:41:04, battre wrote: > return 0 Done.
lgtm
rhalavati@chromium.org changed reviewers: + ben@chromium.org
ben@chromium.org: Please review changes in src/BUILD.gn We have added a tool to extract network traffic annotations and it is added to the src/BUILD.gn
lgtm
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, msramek@chromium.org, pastarmovj@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2448133006/#ps890001 (title: "Comments addressed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 890001, "attempt_start_ts": 1492752337541490, "parent_rev": "61f066322d740afe3e2b17789fbc8960ed56558b", "commit_rev": "3051a40f7c5567d6bbee847227e7a31880fad143"}
Message was sent while issue was closed.
Description was changed from ========== Tool added to extract network traffic annotations. This is a clang tool that extracts all occurrences of network traffic annotattions and produces a report of annotated locations and text. BUG=656607 ========== to ========== Tool added to extract network traffic annotations. This is a clang tool that extracts all occurrences of network traffic annotattions and produces a report of annotated locations and text. BUG=656607 Review-Url: https://codereview.chromium.org/2448133006 Cr-Commit-Position: refs/heads/master@{#466281} Committed: https://chromium.googlesource.com/chromium/src/+/3051a40f7c5567d6bbee847227e7... ==========
Message was sent while issue was closed.
Committed patchset #45 (id:890001) as https://chromium.googlesource.com/chromium/src/+/3051a40f7c5567d6bbee847227e7... |