Various cleanups and enhancements #7

jkuebart · 2018-07-28T17:39:46Z

Only a single PR since several changes have been submitted at once.

I created the »base« branch in order to separate previously reviewed changes. Going forward, it would be useful to retire branches once their PRs and reviews are finished.

Pass -pthread for compilation and linking as this is a more general way to build multi-threaded code than just linking to -lpthread. Fix build of lua and luac (if uncommented) by using identical environments for all compiler invocations.

Add linker flags to export dynamic symbols. Preprocessor macros and libraries are taken from Lua's Makefile. Without LUA_USE_DLOPEN, the following is not supported: package.loadlib("abc", "def") Fix POSIX build of lua and luac by linking against -lm.

…uautils.h

…rdcoded lua utility functions

jkuebart · 2018-07-28T17:42:02Z

src/luautils.h

+#include <string>
+#include "lua/src/lua.hpp"
+
+using namespace std;


»using namespace« in a header file is a no-go: there is no way to undo the effect for its users and it's bound to hide unwanted dependencies between headers: in this case, stringutls.h has errors unless this header is included first because it also uses std:: symbols unprefixed.

Unfortunately, due to the lack of a module system there is no way to avoid namespace prefixes at least in headers.

jkuebart · 2018-07-28T17:42:48Z

src/luautils.h

+};
+
+struct StringPtr{
+	StringPtr(const char* c_, const size_t len_):c(c_),len(len_){}


std::size_t (also below)

also, using a different name for c'tor arguments is common, but unnecessary. It is therefore entirely a question of style to write

StringPtr(const char* c, const std::size_t len) : c(c) , len(len) {}

I think a constructor taking a std::string would be beneficial for this class:

explicit StringPtr(const std::string& s) : c(s.data()) , len(s.size()) {}

Main purpose of this class is to hand c-strings to the overloaded lua_push function as single arguments. As there is also a lua_push that takes std::strings, the StringPtr would not be needed there. Do y... Would there still be a use for the suggested constructor? To make StringPtr more general maybe?

It was intended for the call lua_pushfield(L, rule) in main.cpp line 92, but by the time I got there I realised it could just pass the std::string. So you're right, it would actually be confusing to have a std::string c'tor and this struct should stay as it is ;-)

jkuebart · 2018-07-28T17:44:50Z

src/luautils.h

+
+struct StringPtr{
+	StringPtr(const char* c_, const size_t len_):c(c_),len(len_){}
+	const char* c;


In order to be consistent with the const member len, this should be

const char* const c;

jkuebart · 2018-07-28T17:49:37Z

src/luautils.h

+void lua_push(lua_State *L, const int value){
+	lua_pushinteger(L, value);
+}
+void lua_push(lua_State *L, const string value){


Use std::string and const std::string& for efficiency.

jkuebart · 2018-07-28T17:51:52Z

src/luautils.h

+	lua_pushinteger(L, value);
+}
+void lua_push(lua_State *L, const string value){
+	lua_pushstring(L, value.c_str());


It's more efficient to use

lua_pushlstring(L, value.data(), value.size());

Since the length is already known, there is no need for lua_pushstring to walk the string again.

Using .data() instead of .c_str() avoids a potential unnecessary copy in order to add a terminating '\0'

If the string contains embedded '\0' characters, the original version only copies part of the string.

jkuebart · 2018-07-28T18:10:40Z

src/main.cpp

 #endif
 	return result;
 }
+


spurious line

jkuebart · 2018-07-28T18:11:45Z

src/stringutils.h

+
+#pragma once
+
+#include <iostream>


ostream is sufficient.

jkuebart · 2018-07-28T18:12:08Z

src/stringutils.h

+	const char* s;
+	size_t num;
+};
+ostream& operator<< (ostream& stream, const repeat rep){


const repeat& is more efficient.

use std::ostream

jkuebart · 2018-07-28T18:14:00Z

src/stringutils.h

+}
+
+// shorten string and remove line breaks and tabs
+string shorten(const string& txt, const size_t numchars){


use std::string here and below.

jkuebart · 2018-07-28T18:14:17Z

test/calculator.lua

-	print("                          ", dump(params))
 	return params.matched
 end



two spurious lines before EOF

jkuebart · 2018-07-28T18:21:03Z

src/luautils.h

+}
+
+auto lua_loadutils(lua_State *L){
+	const auto utils = R"(


indentation looks off

jkuebart · 2018-07-28T18:23:26Z

test/calculator.lua

 	end
 end

 function Num(params)


This function can go.

jkuebart · 2018-07-28T18:30:28Z

These commits also contain several whitespace problems, such as indent with spaces in peglib.h, trailing whitespace in main.cpp and peglib.h and trailing newlines in calculator.lua, luautils.h and main.cpp

jkuebart · 2018-07-28T18:31:54Z

src/main.cpp

+		cerr << "error loading \"" << args[2] << "\": file not found" << endl;
 		return EXIT_FAILURE;
-	}		
+    }


indentation looks off

…rmediate 0 characters

…ser construction

…icient

jkuebart and others added 4 commits July 23, 2018 16:46

Support dynamic module loading.

87cea54

Add linker flags to export dynamic symbols. Preprocessor macros and libraries are taken from Lua's Makefile. Without LUA_USE_DLOPEN, the following is not supported: package.loadlib("abc", "def") Fix POSIX build of lua and luac by linking against -lm.

refactory: cleaning and generalizing lua_push, moved lua utils into l…

5eb4834

…uautils.h

cleaned parser debug output, put string utilities in extra header, ha…

0a27cf4

…rdcoded lua utility functions

jkuebart commented Jul 28, 2018

View reviewed changes

src/luautils.h

}

auto lua_loadutils(lua_State *L){

const auto utils = R"(

Copy link

Collaborator Author

jkuebart Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation looks off

jkuebart commented Jul 28, 2018

View reviewed changes

test/calculator.lua Outdated

end

end

function Num(params)

Copy link

Collaborator Author

jkuebart Jul 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can go.

jkuebart commented Jul 28, 2018

View reviewed changes

mqnc added 18 commits July 30, 2018 16:15

merged base branch into master

0cde2f5

Merge branch 'master' into develop

e3ef871

removed using namespace std from headers

411aff6

size_t -> std::size_t

6588df6

made string parameters more efficient

e4efd48

made lua_push for std::strings more efficient and compliant with inte…

23b164b

…rmediate 0 characters

passing parameters to lua_push by ref for efficiency

a9d2c4a

made functions inline

002d67b

loading lua utils before parser so functions are available during par…

31a7959

…ser construction

fixed bug with declaration

1a29df2

passing string more efficiently

a16901a

made LuaStackPtr ctor explicit

208c762

parametrized debug text length, made debug output marginally more eff…

d8dd716

…icient

removed Num function

24b60c3

made string shortening unnoticably more efficient in general

5652ac5

cleaned whitespaces

07d8116

cleaned whitespaces again

816aa25

popping debug string from lua stack after fetching it

acab73a

mqnc closed this Jul 30, 2018

mqnc deleted the develop branch December 26, 2018 22:51


		#pragma once

		#include <iostream>

Various cleanups and enhancements #7

Various cleanups and enhancements #7

Uh oh!

Conversation

jkuebart commented Jul 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkuebart commented Jul 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants