Default compiler standard #1720

bruntib · 2018-08-27T23:52:40Z

No description provided.

gyorb · 2018-08-31T07:58:30Z

libcodechecker/analyze/log_parser.py

+        test_file = temp_file + ('.c' if lang == 'c' else '.cpp')
+        os.rename(temp_file, test_file)
+
+        with open(test_file, 'w') as f:


Please use io.open

gyorb · 2018-08-31T08:00:09Z

libcodechecker/analyze/log_parser.py

+        executable = os.path.join(tempfile.gettempdir(),
+                                  next(tempfile._get_candidate_names()))
+
+        proc = subprocess.Popen([compiler, '-o', executable, test_file],


Please handle Popen exceptions in case the call fails.

gyorb · 2018-08-31T08:00:14Z

libcodechecker/analyze/log_parser.py

+
+        os.chmod(executable, stat.S_IXUSR)
+
+        proc = subprocess.Popen([executable], stdout=subprocess.PIPE)


Please handle Popen exceptions in case the call fails.

gyorb · 2018-08-31T08:14:14Z

libcodechecker/analyze/log_parser.py

-                                           compiler_includes_dump_file))
-        remove_file_if_exists(os.path.join(output_path,
-                                           compiler_target_dump_file))
+        global compiler_info_dump_file


I'm not a big fan of global variables, could you change it?

I agree. I tried some other implementations, but every other solution was much uglier and boilerplate. So I left it like this, maybe we could figure out something more elegant later.

gyorb · 2018-08-31T08:15:54Z

libcodechecker/analyze/log_parser.py

+        proc = subprocess.Popen([executable], stdout=subprocess.PIPE)
+        out, _ = proc.communicate()
+
+        os.remove(test_file)


Please handle exceptions if the files are already removed for some reason, or use the remove_file_if_exists function.

gyorb · 2018-08-31T08:22:31Z

libcodechecker/analyze/log_parser.py

+}
+    """
+
+    err = ""


Why is this named err, the cpp standard argument will be stored in it.

csordasmarton · 2018-09-03T10:08:01Z

libcodechecker/analyze/log_parser.py

+    if parseLogOptions.compiler_info_file is None:
+        _, temp_file = tempfile.mkstemp()
+        test_file = temp_file + ('.c' if lang == 'c' else '.cpp')
+        os.rename(temp_file, test_file)


I think removing a temp file should be done in the following form:

_, temp_file = tempfile.mkstemp() try: ... finally: remove_file_if_exists(temp_file) remove_file_if_exists(test_file)

What do you think @gyorb?

Using NamedTemporaryFile in a with statement with a suffix (no renaming is needed) would be easier and it would remove the file when it is out of context.

gyorb

Please update the temporary file handling.

whisperity · 2018-09-04T08:31:01Z

libcodechecker/libhandlers/analyze.py

@@ -127,16 +127,24 @@ def add_arguments_to_parser(parser):
                        dest="compiler_includes_file",
                        required=False,
                        default=None,
-                        help="Read the compiler includes from the specified "
-                             "file rather than invoke the compiler "
+                        help="DEPRECATED. Read the compiler includes from the "


Deprecated arguments used to have a wrapper somewhere which allowed setting them to behave with emitting a warning that a deprecated argument is used. I think this should be introduced again, or just simply the argument parser round (__handle) written in a way that the user is warned about the deprecated argument.

I'll try to search this wrapper in the git history. However, I'd propose a new policy rule: if there are such changes which involve other parts of the source code requiring their change, and not in the scope of the current pull request, then a new issue should be registered, maybe by additionally providing the assignee. What do you think?

codechecker/libcodechecker/libhandlers/check.py

Line 56 in 4515730

class DeprecatedOptionAction(argparse.Action):

Although yeah, the handling of this should be made more reusable and standard.
(Then again, argparse should also be extended to allow broader cross-argument rules and dependencies for which we use __handle...)

whisperity

Are you sure actually compiling something and executing it is the best approach? We could simply use a #pragma GCC error preprocessor macro (both Clang and MSVC understand it too!) and splitting the compiler stderr could also give us the information.

whisperity · 2018-09-04T08:31:56Z

libcodechecker/analyze/log_parser.py

+                          "detector.")
+
+        if os.path.isfile(exe):
+            os.chmod(exe, stat.S_IXUSR)


Will the user always have the permission to set these bits?

The idea of using preprocessing time error sounds good. Do you know any difference between #error and #pragma GCC error? Isn't the former one more standard?

The #pragma version gives me different outputs with different compilers. g++ (5.5.0) seems not to understand it: error: invalid "#pragma GCC error" directive.
By the way can we rely on the error message format between different compilers, or even same compiler and different versions? Wouldn't it complicate the parsing of this output?

I didn't know about #error.

The output should be sufficiently small: CC_FOUND_STARDARD_VER|23. On that, a simple regex can give us the answer, irrespective to whatever specific compiler and version is used.

whisperity · 2018-09-04T08:33:59Z

Also, with regards to the temporary file: we could distribute the "prepared" C/C++ code as a resource with the CodeChecker install, instead of creating them at runtime.

bruntib · 2018-09-04T09:59:01Z

I tried to keep these C++ codes in separate files but it was not possible to reach them from the test code.

whisperity · 2018-09-04T10:05:59Z

@bruntib That is an ongoing issue (that the tests can rarely pull configuration and other stuff from the deployed environment), see #787.

Until then, I think still putting these into separate files (e.g. config or vendor?) and making the make test command unconditionally copy them over to some well-defined location is a good workaround. When we have some time, as part of #787, we will figure something better out.

bruntib · 2018-09-04T10:22:34Z

The idea behind this solution was that log_parser.py highly depends on these .cpp files, so delivering them as a vendor would be too far. I think this can also be a reason not to confide the build system managing the file locations. So couldn't we consider this current solution as a workaround?
By the way the problem was in the unit tests of log_parser. It can't (and wouldn't be logical either) to make it rely on a CodeChecker package to find these C++ files through config options.

whisperity · 2018-09-04T10:49:19Z

Was just an idea, this workaround is good enough for me. However, I think it would be better if the code, if it stays in the Python file, gets moved out of the function's scope? It is... essentially a static const of this function, it is a resource, it is also very long code, it should reside amongst some top-level declarations.

bruntib · 2018-09-04T11:45:37Z

The outer scope is global scope which we try to fight against, even in this ticket for another variable. Would you suggest to place it there? By the way in an other language I would choose static const as it supports the rule of thumb of keeping the variables in the innermost possible scope. We have even a checker for this :)

dkrupp

gcc accepts -std=c++11 in the following forms
-std=c++11
--std=c++11
--std c++11

Only the first form is handled.
Please handle the other variations too.

dkrupp · 2018-09-06T09:12:31Z

libcodechecker/analyze/analyzers/analyzer_clang_tidy.py

@@ -136,6 +136,10 @@ def construct_analyzer_cmd(self, result_handler):

            analyzer_cmd.extend(self.buildaction.compiler_includes)

+            if not next((x for x in analyzer_cmd if x.startswith('-std=')),


gcc accepts -std=c++11 in the following forms
-std=c++11
--std=c++11
--std c++11

Only the first form is handled.
Please handle the other variations too.

The --compiler-includes and --compiler-target flags are deprecated under "CodeChecker analyze" command, --compiler-info is introduced instead. The new compiler info dump file contains the information of these two merged into one JSON, however, the files with the old format still can be used for supporting reverse compatibility.

csordasmarton

Please extend the New features tab with this feature:

C/C++ standard auto detection
Detect automatically which C/C++ standard was used for compilation by gcc and pass the relevant option to Clang (e.g. -std=c++11)

Some implicit information, like implicit include paths or compiler targets, may depend on the used C/C++ language standard. This can be provided by "-std=" flag. However, in some cases this flag is not used and has a default value which depends on the compiler binary. In this commit the intention is to detect this default language standard version.

bruntib added enhancement 🌟 analyzer 📈 Related to the analyze commands (analysis driver) labels Aug 27, 2018

bruntib added this to the release 6.8 milestone Aug 27, 2018

gyorb suggested changes Aug 31, 2018

View reviewed changes

bruntib force-pushed the default_compiler_standard branch from 1b2bec4 to b6c960c Compare August 31, 2018 21:21

bruntib requested a review from gyorb August 31, 2018 21:22

csordasmarton reviewed Sep 3, 2018

View reviewed changes

gyorb suggested changes Sep 3, 2018

View reviewed changes

bruntib force-pushed the default_compiler_standard branch from b6c960c to eeebd03 Compare September 3, 2018 16:34

bruntib requested review from gyorb and csordasmarton September 3, 2018 16:58

whisperity reviewed Sep 4, 2018

View reviewed changes

bruntib force-pushed the default_compiler_standard branch from eeebd03 to 23de4b8 Compare September 4, 2018 16:00

dkrupp requested changes Sep 6, 2018

View reviewed changes

bruntib force-pushed the default_compiler_standard branch 3 times, most recently from 9ed0aa6 to 217754b Compare September 6, 2018 12:11

bruntib requested a review from dkrupp September 6, 2018 12:12

bruntib force-pushed the default_compiler_standard branch from 217754b to 37ce3ec Compare September 6, 2018 13:02

bruntib force-pushed the default_compiler_standard branch from 37ce3ec to ce6c56f Compare September 7, 2018 07:54

csordasmarton suggested changes Sep 7, 2018

View reviewed changes

bruntib force-pushed the default_compiler_standard branch from ce6c56f to 133b01d Compare September 7, 2018 09:51

bruntib force-pushed the default_compiler_standard branch from 133b01d to 44fc7f7 Compare September 7, 2018 10:17

bruntib force-pushed the default_compiler_standard branch from 44fc7f7 to fcd5e85 Compare September 7, 2018 11:13

dkrupp merged commit aa36dcc into Ericsson:master Sep 7, 2018

bruntib deleted the default_compiler_standard branch September 7, 2018 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default compiler standard #1720

Default compiler standard #1720

bruntib commented Aug 27, 2018

gyorb Aug 31, 2018

gyorb Aug 31, 2018

gyorb Aug 31, 2018

gyorb Aug 31, 2018

bruntib Aug 31, 2018

gyorb Aug 31, 2018

gyorb Aug 31, 2018

csordasmarton Sep 3, 2018

gyorb Sep 3, 2018

gyorb left a comment

whisperity Sep 4, 2018

bruntib Sep 4, 2018

whisperity Sep 4, 2018 •

edited

Loading

whisperity left a comment

whisperity Sep 4, 2018

bruntib Sep 4, 2018

bruntib Sep 4, 2018

whisperity Sep 4, 2018

whisperity commented Sep 4, 2018

bruntib commented Sep 4, 2018

whisperity commented Sep 4, 2018

bruntib commented Sep 4, 2018

whisperity commented Sep 4, 2018

bruntib commented Sep 4, 2018 •

edited

Loading

dkrupp left a comment

dkrupp Sep 6, 2018

csordasmarton left a comment


		os.chmod(executable, stat.S_IXUSR)

		proc = subprocess.Popen([executable], stdout=subprocess.PIPE)

		@@ -136,6 +136,10 @@ def construct_analyzer_cmd(self, result_handler):

		analyzer_cmd.extend(self.buildaction.compiler_includes)

		if not next((x for x in analyzer_cmd if x.startswith('-std=')),

Default compiler standard #1720

Default compiler standard #1720

Conversation

bruntib commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyorb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

whisperity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented Sep 4, 2018

bruntib commented Sep 4, 2018

whisperity commented Sep 4, 2018

bruntib commented Sep 4, 2018

whisperity commented Sep 4, 2018

bruntib commented Sep 4, 2018 • edited Loading

dkrupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csordasmarton left a comment

Choose a reason for hiding this comment

whisperity Sep 4, 2018 •

edited

Loading

bruntib commented Sep 4, 2018 •

edited

Loading