Issue 103230043: Eweasel code analysis support
DescriptionEweasel code analysis support
Patch Set 1 #
Total comments: 51
Patch Set 2 : Addressed the first batch of issues raised in the code review. #Patch Set 3 : Final changes, everything should be complete. #
MessagesTotal messages: 7
https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_constants.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:2: description: "Summary description for {EW_SHARED_CODE_ANALYSIS_CONSTANTS}." Better header comment. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:19: Analysis_clean_message: STRING = "No issues." Use `ok' instead of clean. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:52: if a_type ~ Error_short then To compare strings, it is best to use `same_string' rather than ~. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:66: short_violation_type (a_type: STRING): STRING This routine does not seem to be used. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_process.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_process.e:13: rename Wrong indenting. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_result.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:33: analysis_clean: BOOLEAN Use `ok' instead of `clean'. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:36: has_violations: BOOLEAN I think the `check' should be transformed to a ensure clause. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:80: l_exactly_one_analysis_status := (l_count = 1) I think it would be better to write: if Precursor and not eweasel_parse_error then if not_run then l_count := l_count + 1 end if analysis_clean then l_count := l_count + 1 end if has_violations then l_count := l_count + 1 end Result := l_count = 1 end https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:158: set_analysis_clean Use comments and postconditions in the following routines. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_result_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result_inst.e:85: -- inst_initialize (args: STRING) Remove commented code. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result_inst.e:304: Clean_result: STRING = "clean" Shouldn't we use `ok' like for the other compilation instructions when there are no warnings/errors? https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_violation.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:129: (other.type = Unknown_violation_type or else type.is_equal (other.type)) and It would be better to use `same_string' instead of =, ~ or is_equal when comparing strings. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:136: type.is_equal (other.type) and message.is_equal (other.message) Use `same_string' for string comparison. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:139: -- TODO: Triggers an incorrect self-comparison violation (CA071). Create test! We may want to remove that comment for the time being and create the test. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:156: type = is_valid_violation_type (type) It should raise a VWEQ warnign since you are comparing a BOOLEAN with a STRING. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_run_c... File source/code_analysis/ew_run_code_analysis_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_run_c... source/code_analysis/ew_run_code_analysis_inst.e:13: redefine Indentation https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... File source/compilation/ew_eiffel_compilation_result.e (right): https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:180: is_prefix (Exception_occurred_prefix, line) then I would prefer seing: elseif is_prefix (Excption_prefix, line) then analyze_exception_line (line) elseif is_prefix (Exception_occurred_prefix, line) analyze_exception_occurred_line (line) https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:284: analyze_exception (line: STRING) Remove based on above comment. https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:294: analyze_exception_line (line: STRING) Add precondition is_prefix (exception_prefix, line). https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:307: analyze_exception_occurred_line (line: STRING) Add precondition is_prefix (exception_occurred_prefix, line). https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... File source/instructions/ew_start_compile_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... source/instructions/ew_start_compile_inst.e:65: compilation: like compilation_type; Why did you make it an attribute where it is only used in `execute' so a local is ok. https://codereview.appspot.com/103230043/diff/1/source/shared/ew_keyword_const.e File source/shared/ew_keyword_const.e (right): https://codereview.appspot.com/103230043/diff/1/source/shared/ew_keyword_cons... source/shared/ew_keyword_const.e:19: Code_analysis_keyword: STRING = "run_code_analysis" I would call the instructions: - analyze_code - analyze_code_result to be consistent with the other instructions. https://codereview.appspot.com/103230043/diff/1/source/utilities/ew_string_ut... File source/utilities/ew_string_utilities.e (right): https://codereview.appspot.com/103230043/diff/1/source/utilities/ew_string_ut... source/utilities/ew_string_utilities.e:337: safe_string (a_string: STRING): STRING I don't think this is good to create an empty string to make things void-safe. I would ensure that `a_string' is properly checked. For example in EW_CODE_ANALYSIS_VIOLATION you have in `summary' Result.append (safe_string (type)) You could use Result.append_string (type) instead.
Sign in to reply to this message.
One thing that I thought of. It might be better to say in the tcf analyze_code CA001 analyze_result ... The way I see it, we are going to write tests to validate some CAxxx rules, not all the rules. So this would be my recommendation. Now for rules that have parameters, it might be better to find something else than loading from the preferences as this makes it complicated to write tests if we have to edit some xml files. My suggestion would be to have: analyze_code CA011:nb_arguments=5,nb_locals=4 CA080 Here we say to check both CA011 and CA080 and for CA011 we use a specific value for the parameters. What do you think?
Sign in to reply to this message.
Thank you for reviewing my code. Open points: -> 'ok' vs 'clean', please see class `EW_CODE_ANALYSIS_RESULT_INST' -> removing `short_violation_type' function -> reviewing new invariant in class `EW_CODE_ANALYSIS_RESULT' -> attribute instead of local in class `EW_START_COMPILE_INST' -> reviewing new implementation of `is_less' in class `EW_STRING_UTILITIES' Paolo https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_constants.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:2: description: "Summary description for {EW_SHARED_CODE_ANALYSIS_CONSTANTS}." On 2014/06/09 22:20:17, Manus wrote: > Better header comment. Done. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:19: Analysis_clean_message: STRING = "No issues." On 2014/06/09 22:20:17, Manus wrote: > Use `ok' instead of clean. Same point as in `EW_CODE_ANALYSIS_RESULT_INST', awaiting your feedback. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:52: if a_type ~ Error_short then On 2014/06/09 22:20:17, Manus wrote: > To compare strings, it is best to use `same_string' rather than ~. Done. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:66: short_violation_type (a_type: STRING): STRING On 2014/06/09 22:20:17, Manus wrote: > This routine does not seem to be used. It's not, but it's the symmetric conversion function to long_violation_type. It's more like a part of a framework, which I would say it's just not used yet. Would you still like me to remove it? https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_process.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_process.e:13: rename On 2014/06/09 22:20:17, Manus wrote: > Wrong indenting. Done. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_result.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:33: analysis_clean: BOOLEAN On 2014/06/09 22:20:17, Manus wrote: > Use `ok' instead of `clean'. Same point as in `EW_CODE_ANALYSIS_RESULT_INST', awaiting your feedback. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:36: has_violations: BOOLEAN On 2014/06/09 22:20:17, Manus wrote: > I think the `check' should be transformed to a ensure clause. Agreed, but this is actually not guaranteed by the implementation of this feature, unless we add a class invariant as well. I am also adding the following class invariant: invariant violation_list_not_empty: attached violations implies not violations.is_empty https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:80: l_exactly_one_analysis_status := (l_count = 1) On 2014/06/09 22:20:17, Manus wrote: > I think it would be better to write: > if Precursor and not eweasel_parse_error then > if not_run then > l_count := l_count + 1 > end > if analysis_clean then > l_count := l_count + 1 > end > if has_violations then > l_count := l_count + 1 > end > Result := l_count = 1 > end Done. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result.e:158: set_analysis_clean On 2014/06/09 22:20:17, Manus wrote: > Use comments and postconditions in the following routines. Ok. There are no comments and postconditions in the corresponding routines in the parent class as well, I am adding them there too. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_result_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result_inst.e:85: -- inst_initialize (args: STRING) On 2014/06/09 22:20:17, Manus wrote: > Remove commented code. Done. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result_inst.e:304: Clean_result: STRING = "clean" On 2014/06/09 22:20:18, Manus wrote: > Shouldn't we use `ok' like for the other compilation instructions when there are > no warnings/errors? I personally find that 'clean' is better. Intuitively, 'clean' means to me that there were no violations, 'ok' would mean that the execution of the analysis was successful (without warnings or errors), regardless of its result. This is exactly what the 'clean' keyword means. You can still have a clean analysis with warnings (thus not quite 'ok'), for example a 'class not found' and a 'rule not found' warning. Right now you write it like this: analyze_code_result clean rule_warning class_warning The following would look confusing to me: analyze_code_result ok rule_warning class_warning This is my opinion, however it is your choice. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_violation.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:129: (other.type = Unknown_violation_type or else type.is_equal (other.type)) and On 2014/06/09 22:20:18, Manus wrote: > It would be better to use `same_string' instead of =, ~ or is_equal when > comparing strings. Done. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:136: type.is_equal (other.type) and message.is_equal (other.message) On 2014/06/09 22:20:18, Manus wrote: > Use `same_string' for string comparison. Done. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:139: -- TODO: Triggers an incorrect self-comparison violation (CA071). Create test! On 2014/06/09 22:20:18, Manus wrote: > We may want to remove that comment for the time being and create the test. Of course, sorry! I uploaded the changes here a bit in a hurry before leaving, so that you could start reviewing them the same day, but this was my plan as well. I will of course clear this and other TODOs in the code (if any). I also have a couple of more similar tests which I want to create (plus the tests for my rules), but I was planning to write them after I am done with the eweasel internal code. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_violation.e:156: type = is_valid_violation_type (type) On 2014/06/09 22:20:18, Manus wrote: > It should raise a VWEQ warnign since you are comparing a BOOLEAN with a STRING. Thank you. It does, but it didn't notice it because of the many other warnings. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_run_c... File source/code_analysis/ew_run_code_analysis_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_run_c... source/code_analysis/ew_run_code_analysis_inst.e:13: redefine On 2014/06/09 22:20:18, Manus wrote: > Indentation Done. https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... File source/compilation/ew_eiffel_compilation_result.e (right): https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:180: is_prefix (Exception_occurred_prefix, line) then On 2014/06/09 22:20:18, Manus wrote: > I would prefer seing: > elseif is_prefix (Excption_prefix, line) then > analyze_exception_line (line) > elseif is_prefix (Exception_occurred_prefix, line) > analyze_exception_occurred_line (line) > Done. https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:284: analyze_exception (line: STRING) On 2014/06/09 22:20:18, Manus wrote: > Remove based on above comment. Done. https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:294: analyze_exception_line (line: STRING) On 2014/06/09 22:20:18, Manus wrote: > Add precondition is_prefix (exception_prefix, line). Done. https://codereview.appspot.com/103230043/diff/1/source/compilation/ew_eiffel_... source/compilation/ew_eiffel_compilation_result.e:307: analyze_exception_occurred_line (line: STRING) On 2014/06/09 22:20:18, Manus wrote: > Add precondition is_prefix (exception_occurred_prefix, line). Done. https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... File source/instructions/ew_start_compile_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... source/instructions/ew_start_compile_inst.e:65: compilation: like compilation_type; On 2014/06/09 22:20:18, Manus wrote: > Why did you make it an attribute where it is only used in `execute' so a local > is ok. I thought it would be necessary to access it from subclasses (EW_RUN_CODE_ANALYSIS_INST in this case). In the end it was not, but there is the possibility that future changes to that or other class might need it. In the end it is not needed right now, so would you like me to revert it to a local? https://codereview.appspot.com/103230043/diff/1/source/shared/ew_keyword_const.e File source/shared/ew_keyword_const.e (right): https://codereview.appspot.com/103230043/diff/1/source/shared/ew_keyword_cons... source/shared/ew_keyword_const.e:19: Code_analysis_keyword: STRING = "run_code_analysis" On 2014/06/09 22:20:18, Manus wrote: > I would call the instructions: > - analyze_code > - analyze_code_result > > to be consistent with the other instructions. Done. https://codereview.appspot.com/103230043/diff/1/source/utilities/ew_string_ut... File source/utilities/ew_string_utilities.e (right): https://codereview.appspot.com/103230043/diff/1/source/utilities/ew_string_ut... source/utilities/ew_string_utilities.e:337: safe_string (a_string: STRING): STRING On 2014/06/09 22:20:18, Manus wrote: > I don't think this is good to create an empty string to make things void-safe. I > would ensure that `a_string' is properly checked. For example in > EW_CODE_ANALYSIS_VIOLATION you have in `summary' > > Result.append (safe_string (type)) > > You could use > > Result.append_string (type) > > instead. Done for `summary'. However `is_less' is trickier. Please review my new solution for is_less, which I will upload as soon as possible.
Sign in to reply to this message.
On 2014/06/10 05:54:50, Manus wrote: > One thing that I thought of. It might be better to say in the tcf > > analyze_code CA001 > analyze_result ... > > The way I see it, we are going to write tests to validate some CAxxx rules, not > all the rules. So this would be my recommendation. > > Now for rules that have parameters, it might be better to find something else > than loading from the preferences as this makes it complicated to write tests if > we have to edit some xml files. My suggestion would be to have: > > analyze_code CA011:nb_arguments=5,nb_locals=4 CA080 > > Here we say to check both CA011 and CA080 and for CA011 we use a specific value > for the parameters. > > What do you think? Regarding the first point here, I see your point, one will most of the times want to test a single rule (or perhaps a couple of them) and loading a complex xml file every time just for this would be very inconvenient and hard to mantain. This is exactly the reason why I implemented the new -caforcerules command line option in Eiffel Inspector as my initiative. However, I don't think that dumping all the other switches would be a good idea. In particular: -> 'loadprefs': this one is probably the least useful, however one could write at least one test testing the preference loading mechanism. -> 'loaddefaults': this switch is useful for testing the default values of rules. A possible use case: writing a test that asserts that a newly created consisting of the auto-generated hello world class and routine should not violate any rules if the defaults are used. Such a test can only be written if this switch exists. -> 'class': this one is the most useful, why would you drop it? A complex test might be made of more than one class, and you might want to analyze a single class only. When you don't need these switch, you can just write "analyze_code rule CA001", which is only one word longer than what you proposed. I can still do it if you wish, but I don't see why you wouldn't drop all these switches, which (except perhaps for loadprefs) don't seem useless to me. As for the second point (specifying rule preferences from the command line), yes, I also think this would be useful. I am now trying to come up with a not-overly-complicated implementation of this in Eiffel Inspector. I will submit it as soon as it's ready. Please give me some feedback here when you have time.
Sign in to reply to this message.
https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_result_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_result_inst.e:304: Clean_result: STRING = "clean" Actually today we have the following for the compiler compile_result ok compile_result validity_error TEST VEEN compile_result validity_warning TEST Unused_local_warning So it is clear that ok is a compilation without warnings or errors. When validity_error is specified it means that the compilation failed with an error and it will need to be fixed before it can compile. And finally with `validity_warning' this is a compilation that has completed with some warnings. So I'm thinking the same here for analyze_code_result. ok means it completed and no warnings. And otherwise we list the various warnings reported by the tool. https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... File source/instructions/ew_start_compile_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... source/instructions/ew_start_compile_inst.e:65: compilation: like compilation_type; Yes, that would be best. https://codereview.appspot.com/103230043/diff/1/source/utilities/ew_string_ut... File source/utilities/ew_string_utilities.e (right): https://codereview.appspot.com/103230043/diff/1/source/utilities/ew_string_ut... source/utilities/ew_string_utilities.e:337: safe_string (a_string: STRING): STRING will do.
Sign in to reply to this message.
Everything done. I have uploaded the latest patch. Please let me know if there are more issues or if I can commit everything. I will commit very soon the first tests. https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... File source/code_analysis/ew_code_analysis_constants.e (right): https://codereview.appspot.com/103230043/diff/1/source/code_analysis/ew_code_... source/code_analysis/ew_code_analysis_constants.e:66: short_violation_type (a_type: STRING): STRING On 2014/06/10 13:25:35, paolanto wrote: > On 2014/06/09 22:20:17, Manus wrote: > > This routine does not seem to be used. > > It's not, but it's the symmetric conversion function to long_violation_type. > It's more like a part of a framework, which I would say it's just not used yet. > Would you still like me to remove it? Discussed, leave it as it is. Added postcondition. https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... File source/instructions/ew_start_compile_inst.e (right): https://codereview.appspot.com/103230043/diff/1/source/instructions/ew_start_... source/instructions/ew_start_compile_inst.e:65: compilation: like compilation_type; On 2014/06/10 17:24:08, Manus wrote: > Yes, that would be best. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
