Factor out language_infot from languaget #2157

peterschrammel · 2018-05-05T17:32:17Z

Prerequisite for #2127 and #2098 to make access to language-specific methods more efficient.

Previously, using language-specific methods such as
from_expr() required creating a new languaget instance
which could be potentially costly.
This PR introduces a language_infot class that offers
language-specific methods that don't require parsing
functionality. language_infot is now used for language
registration (instead of language_entryt). The immutable
registered *_language_infot instances are shared among the
corresponding languaget instances that are requested by
the get_language_from functions.

tautschnig

I decided to ignore the don't-review-request... Just one genuine error spotted, otherwise nit-picks.

tautschnig · 2018-05-05T17:39:47Z

src/clobber/CMakeLists.txt

@@ -25,8 +25,6 @@ target_link_libraries(clobber-lib
 )

 add_if_library(clobber-lib bv_refinement)
-add_if_library(clobber-lib specc)
-add_if_library(clobber-lib php)


Wishlist: Could those changes (and related ones) go in a separate commit, please?

tautschnig · 2018-05-05T17:40:18Z

src/clobber/clobber_parse_options.cpp

@@ -38,8 +38,6 @@ Author: Daniel Kroening, [email protected]

 #include <cbmc/version.h>

-// #include "clobber_instrumenter.h"
-


Maybe a "Clobber: cleanup" commit?

Moved into a separate PR: #2159

tautschnig · 2018-05-05T17:41:00Z

src/goto-analyzer/CMakeLists.txt

@@ -23,8 +23,6 @@ target_link_libraries(goto-analyzer-lib

 add_if_library(goto-analyzer-lib java_bytecode)
 add_if_library(goto-analyzer-lib jsil)
-add_if_library(goto-analyzer-lib specc)
-add_if_library(goto-analyzer-lib php)


To go in the cleanup commit.

tautschnig · 2018-05-05T17:41:20Z

src/clobber/Makefile

-  OBJ += ../php/php$(LIBEXT)
-  CP_CXXFLAGS += -DHAVE_PHP
-endif
-


Separate commit (see above).

tautschnig · 2018-05-05T17:42:19Z

src/ansi-c/ansi_c_language_info.h

+  }
+  std::string description() const override
+  {
+    return "ANSI-C 99";


Correct copy&paste, but we have meanwhile gone well beyond "ANSI" C.

tautschnig · 2018-05-05T17:46:54Z

src/jsil/jsil_language_info.h

+
+  irep_idt id() const override
+  {
+    return ID_C;


Copy&paste error: "jsil" (should probably introduce an ID_jsil).

tautschnig · 2018-05-05T17:47:09Z

src/jsil/jsil_language_info.h

+  }
+  std::string description() const override
+  {
+    return "ANSI-C 99";


Copy&paste error, was "Javascript Intermediate Language"

tautschnig · 2018-05-05T17:48:31Z

src/langapi/language_info.h

+  virtual std::set<std::string> extensions() const
+  {
+    return std::set<std::string>();
+  }


Shouldn't these also be = 0? Those default implementations make no sense.

tautschnig · 2018-05-05T17:50:41Z

unit/analyses/dependence_graph.cpp

+#include <util/arith_tools.h>
+#include <util/c_types.h>
+#include <util/std_code.h>
+#include <util/symbol_table.h>


Nit pick: I'm not sure this is a value-adding change? We usually do grouping by directories.

tautschnig · 2018-05-05T17:51:02Z

unit/java_bytecode/java_object_factory/gen_nondet_string_init.cpp

+#include <testing-utils/catch.hpp>
+#include <util/expr.h>
+#include <util/namespace.h>
+#include <util/std_code.h>


As above: does this add value?

peterschrammel · 2018-05-07T10:08:53Z

@tautschnig, this is ready for re-review.

peterschrammel · 2018-05-07T10:09:00Z

The TG bump needs to pass before merging this.

tautschnig · 2018-05-07T10:21:19Z

src/cbmc/cbmc_languages.cpp

-#include <ansi-c/ansi_c_language.h>
-#include <cpp/cpp_language.h>
+#include <ansi-c/ansi_c_language_info.h>
+#include <cpp/cpp_language_info.h>

 #ifdef HAVE_JSIL


Would you be ok to add a commit to this PR that removes all occurrences of HAVE_JSIL from the codebase? It's not used consistently and thus doesn't really make sense. (Else it can happen in some later PR, once this is merged.)

tautschnig · 2018-05-07T10:24:04Z

src/langapi/language_info.h

+    std::string &name,
+    const namespacet &ns) const;
+
+  virtual ~language_infot()


Nit pick: you might use = default;

peterschrammel · 2018-05-07T23:08:04Z

@tautschnig, please check whether the last commit is what you intended.

tautschnig · 2018-05-07T23:13:49Z

@tautschnig, please check whether the last commit is what you intended.

Yes, thank you very much!

peterschrammel · 2018-05-08T00:42:05Z

TG bump passing...

thk123

Just some minor changes, no need for re-review, will approve once I've checked TG bump

thk123 · 2018-05-08T08:23:36Z

src/ansi-c/ansi_c_language_info.cpp

+
+Module:
+
+Author: Daniel Kroening, [email protected]


Should this be Diffblue ltd?

If it is a new file written by a Diffblue employee, yes.

thk123 · 2018-05-08T08:30:07Z

src/cpp/cpp_language_info.h

+  {
+  }
+
+  // conversion from expression into string


Either remove or turn into doxygen comment

thk123 · 2018-05-08T08:31:55Z

src/java_bytecode/java_bytecode_language_info.cpp

+
+Module: Java Bytecode Language
+
+Author: Peter Schrammel


And this should probably also be Diffblue ltd?

thk123 · 2018-05-08T08:32:42Z

src/langapi/language_info.h

+  /// \param ns: a namespace
+  /// \return false if conversion succeeds
+  virtual bool
+  from_expr(const exprt &expr, std::string &code, const namespacet &ns) const;


Why not = 0?

Yes, that could be considered.

thk123 · 2018-05-08T08:33:06Z

src/langapi/language_info.h

+public:
+  // language id / description
+
+  virtual irep_idt id() const = 0;


Please document these

thk123 · 2018-05-08T08:35:21Z

src/langapi/mode.cpp

-  for(const auto &language : languages)
-    if(mode == language.mode)
-      return language.factory();
+  for(const auto &language_info : languages)


Inner content spread over multiple lines so please wrap in braces

thk123 · 2018-05-08T08:35:36Z

src/langapi/mode.cpp

+/// \return the language info (assumes that the language has been registered)
+const language_infot &get_language_info_from_mode(const irep_idt &mode)
+{
+  for(const auto &language_info : languages)


Inner content spread over multiple lines so please wrap in braces

Also mild preference to use above method

thk123 · 2018-05-08T08:37:29Z

src/langapi/mode.cpp

    return nullptr;

  std::string extension=
    std::string(filename, ext_pos+1, std::string::npos);

-  if(extension=="")
+  if(extension.empty())


Random fixes makes this long commit even harder to review - please avoid in future 🙂

thk123 · 2018-05-08T08:39:42Z

src/langapi/mode.cpp

-        e_it++)
-      if(_stricmp(extension.c_str(), e_it->c_str())==0)
-        return l_it->factory();
+    for(const auto &ext : extensions)


to avoid having two variables called extension (one abbreviated) could you rename either the original or the new one to something more descriptive? (e.g. language_extension vs file_extension perhaps?)

thk123

TG pointer looks OK too

chrisr-diffblue

Looks like a reasonable cleanup to me.

martin-cs

Perhaps I am being unobservant but it seems like this could also tidy up some of the expr2c, expr2java, expr2internalstringinmadeuplanguage, expr2yetanotherformatbutwithdifferentprecedence mess.

martin-cs · 2018-05-08T13:07:38Z

src/cbmc/CMakeLists.txt

@@ -16,6 +16,7 @@ target_link_libraries(cbmc-lib
    goto-instrument-lib
    goto-programs
    goto-symex
+    jsil


The "remove conditional inclusion of JSIL" patch seems to be unrelated to the PR. It may well be a good idea (Do we need conditional compilation of features? If so, what is the policy?) but at the moment it is not explained or justified.

See thread above - requested by @tautschnig due to inconsistent use.

I agree that it would better fit a separate PR, but that PR would necessarily depend on this one because of overlapping changes, hence adding it in here seemed ok to me.

martin-cs · 2018-05-08T13:09:35Z

src/ansi-c/ansi_c_language_info.cpp

+
+Module:
+
+Author: Daniel Kroening, [email protected]


If it is a new file written by a Diffblue employee, yes.

martin-cs · 2018-05-08T13:19:14Z

src/langapi/mode.cpp

+  }
+
+  INVARIANT(false, "unregistered language `" + id2string(mode) + "'");
+}


Unreachable? Is this really an invariant?

You may call it that way, but I'd like to attach an error message to it ;)

Previously, using language-specific methods such as from_expr() required creating a new languaget instance which could be potentially costly. This commit introduces a language_infot class that offers language-specific methods that don't require parsing functionality. language_infot is now used for language registration (instead of language_entryt). The immutable registered *_language_infot instances are shared among the corresponding *_languaget instances that are requested by the get_language_from_* functions.

mgudemann

LGTM

kroening · 2018-05-08T14:07:39Z

langapi is dead and deprecated!

smowton

Looks great, thanks for making the effort!

smowton · 2018-05-08T14:44:06Z

@kroening how would you prefer to do a language-specific thing, like "for the language relating to this symbol, get usual file extension" for example?

peterschrammel · 2018-05-08T17:07:33Z

@smowton, the suggestion is to move the language-specific dispatch from the global language registry to the driver program. I.e. everything that requires something language-specific will be parametrised by a function that performs the language-specific dispatch. The presumption is that this will only be necessary in very few cases (show_goto_trace, etc). The advantage is that we can get rid of a global variable.

TGWDB · 2021-02-22T14:24:41Z

This appears to be a nice to do, but is quite out of date. Closing since there seems to be a low likelihood of progress on this at the moment. Encourage the author to rebase and reopen if they would like this merged.

If you believe this closure is erroneous please reopen this PR.

peterschrammel added the do not merge label May 5, 2018

peterschrammel self-assigned this May 5, 2018

peterschrammel requested review from cesaro, chrisr-diffblue, kroening, martin-cs, mgudemann, smowton, tautschnig and thk123 as code owners May 5, 2018 17:32

tautschnig requested changes May 5, 2018

View reviewed changes

peterschrammel force-pushed the language-info branch from 51ea214 to 8f35ddb Compare May 7, 2018 09:46

peterschrammel removed the do not merge label May 7, 2018

peterschrammel force-pushed the language-info branch from 8f35ddb to 37a2317 Compare May 7, 2018 10:02

tautschnig approved these changes May 7, 2018

View reviewed changes

tautschnig reviewed May 7, 2018

View reviewed changes

peterschrammel force-pushed the language-info branch 6 times, most recently from d34a730 to f1fe412 Compare May 7, 2018 22:42

peterschrammel force-pushed the language-info branch from f1fe412 to 5cc171e Compare May 8, 2018 00:38

thk123 reviewed May 8, 2018

View reviewed changes

thk123 approved these changes May 8, 2018

View reviewed changes

peterschrammel force-pushed the language-info branch from 5cc171e to 4cec117 Compare May 8, 2018 09:07

chrisr-diffblue approved these changes May 8, 2018

View reviewed changes

martin-cs approved these changes May 8, 2018

View reviewed changes

peterschrammel added 2 commits May 8, 2018 14:23

Remove conditional inclusion of JSIL

17e1325

peterschrammel force-pushed the language-info branch from 4cec117 to 17e1325 Compare May 8, 2018 13:24

mgudemann approved these changes May 8, 2018

View reviewed changes

kroening added the do not merge label May 8, 2018

smowton approved these changes May 8, 2018

View reviewed changes

peterschrammel mentioned this pull request Sep 6, 2018

Remove deprecated Java and string options [TG-4282] #2907

Merged

TGWDB closed this Feb 22, 2021

TGWDB mentioned this pull request Feb 22, 2021

Json output through languaget interface #2098

Closed

		@@ -38,8 +38,6 @@ Author: Daniel Kroening, [email protected]

		#include <cbmc/version.h>

		// #include "clobber_instrumenter.h"

Factor out language_infot from languaget #2157

Factor out language_infot from languaget #2157

Conversation

peterschrammel commented May 5, 2018 • edited Loading

tautschnig 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

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

peterschrammel commented May 7, 2018

peterschrammel commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterschrammel commented May 7, 2018

tautschnig commented May 7, 2018

peterschrammel commented May 8, 2018

thk123 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

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

thk123 left a comment

Choose a reason for hiding this comment

chrisr-diffblue left a comment

Choose a reason for hiding this comment

martin-cs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgudemann left a comment

Choose a reason for hiding this comment

kroening commented May 8, 2018

smowton left a comment

Choose a reason for hiding this comment

smowton commented May 8, 2018

peterschrammel commented May 8, 2018

TGWDB commented Feb 22, 2021

peterschrammel commented May 5, 2018 •

edited

Loading