Hello,
I get back to the list to continue the discussion.
I like the patch. Did you manage to test it ? I do not know how to check
that errors are caught.
The following code
+ bool valid = (pret == 0);
+ if (!success)
+ valid = false;
should be replaced with
+ bool valid = (pret == 0) && valid;
I would also prefer to have the definition of the struct in multi-line
format.
Concerning the more ambitious FIXME, I have to admit that I have not
researched this at all, and that we can just forget about it for now.
JMarc
-------- Message transféré --------
Sujet : Re: [PATCH] Fix uninitialized variable with wrong type
Date : Fri, 11 Sep 2020 01:29:10 +0300
De : Yuriy Skalko <[email protected]>
Pour : [email protected]
I do not see how returning an int valie which has different interpretations
depending on the OS can be clean. I would propose to change the first element
of cmd_ret with a boolean (if this is enough) or an enum (if we return more
detailed information) and compute this value cleanly in each branch.
Or spend this time implementing the following advice (no idea how easy it is):
// FIXME: replace all calls to RunCommand with ForkedCall
// (if the output is not needed) or the code in ISpell.cpp
// (if the output is needed).
JMarc
I've done some refactoring according to the first advice. Yes, there
should be more separate handling of int/DWORD for different systems. I
think structure will be better here than pair. As I understand, boolean
is enough in this case.
Please check the attached patch.
As for the FIXME comment, I don't know where to get the ISpell.cpp file.
Yuriy
From 749b357e3d9db40a373017124ac097d4253e5e82 Mon Sep 17 00:00:00 2001
From: Yuriy Skalko <[email protected]>
Date: Fri, 11 Sep 2020 00:22:55 +0300
Subject: [PATCH] Refactor runCommand
---
src/Buffer.cpp | 2 +-
src/TextClass.cpp | 2 +-
src/mathed/MathExtern.cpp | 2 +-
src/support/filetools.cpp | 30 +++++++++++++++++-------------
src/support/filetools.h | 2 +-
src/support/os.cpp | 4 ++--
src/support/os_win32.cpp | 4 ++--
7 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 9de281daf3..82fd0899c4 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1357,7 +1357,7 @@ Buffer::ReadStatus Buffer::convertLyXFormat(FileName
const & fn,
LYXERR(Debug::INFO, "Running '" << command_str << '\'');
cmd_ret const ret = runCommand(command_str);
- if (ret.first != 0) {
+ if (!ret.valid) {
if (from_format < LYX_FORMAT) {
Alert::error(_("Conversion script failed"),
bformat(_("%1$s is from an older version"
diff --git a/src/TextClass.cpp b/src/TextClass.cpp
index eb92612d84..4cc5eba0ed 100644
--- a/src/TextClass.cpp
+++ b/src/TextClass.cpp
@@ -92,7 +92,7 @@ bool layout2layout(FileName const & filename, FileName const
& tempfile,
LYXERR(Debug::TCLASS, "Running `" << command_str << '\'');
cmd_ret const ret = runCommand(command_str);
- if (ret.first != 0) {
+ if (!ret.valid) {
if (format == LAYOUT_FORMAT)
LYXERR0("Conversion of layout with layout2layout.py has
failed.");
return false;
diff --git a/src/mathed/MathExtern.cpp b/src/mathed/MathExtern.cpp
index 311b323895..18340e11c8 100644
--- a/src/mathed/MathExtern.cpp
+++ b/src/mathed/MathExtern.cpp
@@ -1026,7 +1026,7 @@ namespace {
<< "\ninput: '" << data << "'" << endl;
cmd_ret const ret = runCommand(command);
cas_tmpfile.removeFile();
- return ret.second;
+ return ret.result;
}
size_t get_matching_brace(string const & str, size_t i)
diff --git a/src/support/filetools.cpp b/src/support/filetools.cpp
index d6b27856cb..ca26b0bde8 100644
--- a/src/support/filetools.cpp
+++ b/src/support/filetools.cpp
@@ -1095,38 +1095,42 @@ cmd_ret const runCommand(string const & cmd)
// (Claus Hentschel) Check if popen was successful ;-)
if (!inf) {
lyxerr << "RunCommand:: could not start child process" << endl;
- return make_pair(-1, string());
+ return {false, string()};
}
- string ret;
+ string result;
int c = fgetc(inf);
while (c != EOF) {
- ret += static_cast<char>(c);
+ result += static_cast<char>(c);
c = fgetc(inf);
}
#if defined (_WIN32)
WaitForSingleObject(process.hProcess, INFINITE);
DWORD pret;
- if (!GetExitCodeProcess(process.hProcess, &pret))
- pret = -1;
+ BOOL success = GetExitCodeProcess(process.hProcess, &pret);
+ bool valid = (pret == 0);
+ if (!success)
+ valid = false;
if (!infile.empty())
CloseHandle(startup.hStdInput);
CloseHandle(process.hProcess);
if (fclose(inf) != 0)
- pret = -1;
+ valid = false;
#elif defined (HAVE_PCLOSE)
int const pret = pclose(inf);
+ bool valid = (pret != -1);
#elif defined (HAVE__PCLOSE)
int const pret = _pclose(inf);
+ bool valid = (pret != -1);
#else
#error No pclose() function.
#endif
- if (pret == -1)
+ if (!valid)
perror("RunCommand:: could not terminate child process");
- return make_pair(pret, ret);
+ return {valid, result};
}
@@ -1174,10 +1178,10 @@ FileName const findtexfile(string const & fil, string
const & /*format*/,
cmd_ret const c = runCommand(kpsecmd);
- LYXERR(Debug::LATEX, "kpse status = " << c.first << '\n'
- << "kpse result = `" << rtrim(c.second, "\n\r") << '\'');
- if (c.first != -1)
- return FileName(rtrim(to_utf8(from_filesystem8bit(c.second)),
"\n\r"));
+ LYXERR(Debug::LATEX, "kpse status = " << c.valid << '\n'
+ << "kpse result = `" << rtrim(c.result, "\n\r") << '\'');
+ if (c.valid)
+ return FileName(rtrim(to_utf8(from_filesystem8bit(c.result)),
"\n\r"));
else
return FileName();
}
@@ -1223,7 +1227,7 @@ bool prefs2prefs(FileName const & filename, FileName
const & tempfile, bool lfun
LYXERR(Debug::FILES, "Running `" << command_str << '\'');
cmd_ret const ret = runCommand(command_str);
- if (ret.first != 0) {
+ if (!ret.valid) {
LYXERR0("Could not run file conversion script prefs2prefs.py.");
return false;
}
diff --git a/src/support/filetools.h b/src/support/filetools.h
index b0f5f7f379..e8fa954851 100644
--- a/src/support/filetools.h
+++ b/src/support/filetools.h
@@ -328,7 +328,7 @@ bool prefs2prefs(FileName const & filename, FileName const
& tempfile,
/// Does file \p file need to be updated by configure.py?
bool configFileNeedsUpdate(std::string const & file);
-typedef std::pair<int, std::string> cmd_ret;
+typedef struct {bool valid; std::string result;} cmd_ret;
cmd_ret const runCommand(std::string const & cmd);
diff --git a/src/support/os.cpp b/src/support/os.cpp
index 616b9c6936..3a5c2e59a2 100644
--- a/src/support/os.cpp
+++ b/src/support/os.cpp
@@ -65,7 +65,7 @@ static string const python23_call(string const & binary, bool
verbose = false)
smatch sm;
try {
static regex const python_reg("\\((\\d*), (\\d*)\\)");
- if (out.first < 0 || !regex_match(out.second, sm, python_reg))
+ if (!out.valid || !regex_match(out.result, sm, python_reg))
return string();
} catch(regex_error const & /*e*/) {
LYXERR0("Regex error! This should not happen.");
@@ -78,7 +78,7 @@ static string const python23_call(string const & binary, bool
verbose = false)
return string();
if (verbose)
- lyxerr << "Found Python " << out.second << "\n";
+ lyxerr << "Found Python " << out.result << "\n";
// Add the -tt switch so that mixed tab/whitespace
// indentation is an error
return binary + " -tt";
diff --git a/src/support/os_win32.cpp b/src/support/os_win32.cpp
index e4f9ebd0fc..536d2a6446 100644
--- a/src/support/os_win32.cpp
+++ b/src/support/os_win32.cpp
@@ -229,8 +229,8 @@ void init(int argc, char ** argv[])
// to the outer split which sets cygdrive with its
// contents until the first blank, discarding the
// unneeded return value.
- if (p.first != -1 && prefixIs(p.second, "Prefix"))
- split(split(p.second, '\n'), cygdrive, ' ');
+ if (p.valid && prefixIs(p.result, "Prefix"))
+ split(split(p.result, '\n'), cygdrive, ' ');
}
}
--
2.28.0.windows.1
--
lyx-devel mailing list
[email protected]
http://lists.lyx.org/mailman/listinfo/lyx-devel
