-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C++ runtime changes for high warning levels #1902
Changes from 17 commits
9845382
092afb2
e030163
2d011c8
eb02a05
4a359c1
70402f8
dde893d
6e46b16
274e3c2
8fd4bcf
577b1d6
8c45d71
aab2c04
63fc7cb
fad0488
fdcfefa
cdfe310
0c4473e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -148,3 +148,4 @@ YYYY/MM/DD, github id, Full name, email | |
2017/05/11, jimallman, Jim Allman, [email protected] | ||
2017/05/26, waf, Will Fuqua, [email protected] | ||
2017/05/29, kosak, Corey Kosak, [email protected] | ||
2017/06/10, jm-mikkelsen, Jan Martin Mikkelsen, [email protected] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
#include "ANTLRErrorListener.h" | ||
|
||
antlr4::ANTLRErrorListener::~ANTLRErrorListener() | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
#include "ANTLRErrorStrategy.h" | ||
|
||
antlr4::ANTLRErrorStrategy::~ANTLRErrorStrategy() | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ using namespace antlr4; | |
DiagnosticErrorListener::DiagnosticErrorListener() : DiagnosticErrorListener(true) { | ||
} | ||
|
||
DiagnosticErrorListener::DiagnosticErrorListener(bool exactOnly) : exactOnly(exactOnly) { | ||
DiagnosticErrorListener::DiagnosticErrorListener(bool exactOnly_) : exactOnly(exactOnly_) { | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
|
||
void DiagnosticErrorListener::reportAmbiguity(Parser *recognizer, const dfa::DFA &dfa, size_t startIndex, size_t stopIndex, | ||
|
@@ -53,7 +53,7 @@ void DiagnosticErrorListener::reportContextSensitivity(Parser *recognizer, const | |
|
||
std::string DiagnosticErrorListener::getDecisionDescription(Parser *recognizer, const dfa::DFA &dfa) { | ||
size_t decision = dfa.decision; | ||
size_t ruleIndex = ((atn::ATNState*)dfa.atnStartState)->ruleIndex; | ||
size_t ruleIndex = (reinterpret_cast<atn::ATNState*>(dfa.atnStartState))->ruleIndex; | ||
|
||
const std::vector<std::string>& ruleNames = recognizer->getRuleNames(); | ||
if (ruleIndex == INVALID_INDEX || ruleIndex >= ruleNames.size()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,32 +21,51 @@ namespace antlr4 { | |
|
||
class ANTLR4CPP_PUBLIC IllegalStateException : public RuntimeException { | ||
public: | ||
IllegalStateException(const std::string &msg = "") : RuntimeException(msg) {}; | ||
IllegalStateException(const std::string &msg = "") : RuntimeException(msg) {} | ||
IllegalStateException(IllegalStateException const&) = default; | ||
~IllegalStateException(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Similar for all other exceptions. However, I wonder why you introduced the d-tor on all the exceptions if you use the default implementation anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in the previous comment. To make sure that the vtable for the class with virtual functions is only generated in one translation unit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird, I never had problems with v-table creation so far. Is this something that changed in a newer compiler? Neither clang nor msc seem to have a problem with that either. Can you point me to an online resource, so I can read up the details? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The vtables are created OK, and will work. The problem is that there are too many of them. That has been part of my coding standard for so long, I assumed everyone did it that way. However, finding an online reference that gives this advice has been surprisingly difficult in about half an hour of searching. The best origin reference I found with a quick look around the office from John Lakos's "Large-Scale C++ Software Design" (from 1996), which has the "minor design rule":
It goes on to describe the reasons, including an anecdote of thousands of instances of the same functions in different translation units caused by duplicated vtables. Locality of reference will probably also suffer (ie. different instances of the same function will lead to less code fitting into the CPU's instruction cache.) (I read that in 1997 and pretty much adopted all of it, so I have probably been doing this for at least 20 years.) Item 24 in Scott Meyers' "More Effective C++" also cautions against inline virtual function definitions with the quote "In large systems, this can lead to programs containing hundreds or thousands of copies of a class's vtbl!" And, of course, Clang has a warning to let you know you're making your object too big:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes a lot of sense. Something new learned. Thanks a lot. |
||
IllegalStateException& operator=(IllegalStateException const&) = default; | ||
}; | ||
|
||
class ANTLR4CPP_PUBLIC IllegalArgumentException : public RuntimeException { | ||
public: | ||
IllegalArgumentException(const std::string &msg = "") : RuntimeException(msg) {}; | ||
IllegalArgumentException(IllegalArgumentException const&) = default; | ||
IllegalArgumentException(const std::string &msg = "") : RuntimeException(msg) {} | ||
~IllegalArgumentException(); | ||
IllegalArgumentException& operator=(IllegalArgumentException const&) = default; | ||
}; | ||
|
||
class ANTLR4CPP_PUBLIC NullPointerException : public RuntimeException { | ||
public: | ||
NullPointerException(const std::string &msg = "") : RuntimeException(msg) {}; | ||
NullPointerException(const std::string &msg = "") : RuntimeException(msg) {} | ||
NullPointerException(NullPointerException const&) = default; | ||
~NullPointerException(); | ||
NullPointerException& operator=(NullPointerException const&) = default; | ||
}; | ||
|
||
class ANTLR4CPP_PUBLIC IndexOutOfBoundsException : public RuntimeException { | ||
public: | ||
IndexOutOfBoundsException(const std::string &msg = "") : RuntimeException(msg) {}; | ||
IndexOutOfBoundsException(const std::string &msg = "") : RuntimeException(msg) {} | ||
IndexOutOfBoundsException(IndexOutOfBoundsException const&) = default; | ||
~IndexOutOfBoundsException(); | ||
IndexOutOfBoundsException& operator=(IndexOutOfBoundsException const&) = default; | ||
}; | ||
|
||
class ANTLR4CPP_PUBLIC UnsupportedOperationException : public RuntimeException { | ||
public: | ||
UnsupportedOperationException(const std::string &msg = "") : RuntimeException(msg) {}; | ||
UnsupportedOperationException(const std::string &msg = "") : RuntimeException(msg) {} | ||
UnsupportedOperationException(UnsupportedOperationException const&) = default; | ||
~UnsupportedOperationException(); | ||
UnsupportedOperationException& operator=(UnsupportedOperationException const&) = default; | ||
|
||
}; | ||
|
||
class ANTLR4CPP_PUBLIC EmptyStackException : public RuntimeException { | ||
public: | ||
EmptyStackException(const std::string &msg = "") : RuntimeException(msg) {}; | ||
EmptyStackException(const std::string &msg = "") : RuntimeException(msg) {} | ||
EmptyStackException(EmptyStackException const&) = default; | ||
~EmptyStackException(); | ||
EmptyStackException& operator=(EmptyStackException const&) = default; | ||
}; | ||
|
||
// IOException is not a runtime exception (in the java hierarchy). | ||
|
@@ -63,12 +82,18 @@ namespace antlr4 { | |
|
||
class ANTLR4CPP_PUBLIC CancellationException : public IllegalStateException { | ||
public: | ||
CancellationException(const std::string &msg = "") : IllegalStateException(msg) {}; | ||
CancellationException(const std::string &msg = "") : IllegalStateException(msg) {} | ||
CancellationException(CancellationException const&) = default; | ||
~CancellationException(); | ||
CancellationException& operator=(CancellationException const&) = default; | ||
}; | ||
|
||
class ANTLR4CPP_PUBLIC ParseCancellationException : public CancellationException { | ||
public: | ||
ParseCancellationException(const std::string &msg = "") : CancellationException(msg) {}; | ||
ParseCancellationException(const std::string &msg = "") : CancellationException(msg) {} | ||
ParseCancellationException(ParseCancellationException const&) = default; | ||
~ParseCancellationException(); | ||
ParseCancellationException& operator=(ParseCancellationException const&) = default; | ||
}; | ||
|
||
} // namespace antlr4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a number of similar changes already and use a simple trailing underscore. Having names like
channel_in
violates the camel case coding style. Can you please change that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update