Third pass at fixing warnings:

- added very stringent checks when compiling under clang; for now, there will still be warnings,
  since I haven't addressed the specific code yet (probably issues in OSX too)
- no issues when compiling with gcc; still need to check VS.

Makefile

@@ -53,6 +53,13 @@ CXXFLAGS+= -Wall -Wextra -Wno-unused-parameter -Wno-ignored-qualifiers
 ifdef HAVE_GCC
   CXXFLAGS+= -Wno-multichar -Wunused -fno-rtti -Woverloaded-virtual -Wnon-virtual-dtor -std=c++14
 endif
+ifdef HAVE_CLANG
+  CXXFLAGS+= -Weverything -Wno-c++17-extensions -Wno-c++98-compat -Wno-c++98-compat-pedantic \
+    -Wno-double-promotion -Wno-switch-enum -Wno-conversion \
+    -Wno-inconsistent-missing-destructor-override \
+    -Wno-exit-time-destructors -Wno-global-constructors -Wno-weak-vtables \
+    -Wno-four-char-constants -Wno-padded
+endif
 
 ifdef PROFILE
   PROF:= -g -pg -fprofile-arcs -ftest-coverage
@@ -63,10 +70,6 @@ else
   endif
 endif
 
-# Even more warnings...
-#CXXFLAGS+= -pedantic -Wpointer-arith -Wcast-qual -Wconversion
-#CXXFLAGS+= -Wshadow -Wimplicit -Wundef -Wnon-virtual-dtor
-#CXXFLAGS+= -Wno-reorder -Wwrite-strings -fcheck-new -Wctor-dtor-privacy
 
 #######################################################################
 # Misc stuff - you should never have to edit this                     #

configure

@@ -386,14 +386,16 @@ cc_check_define __GNUC__ && have_gcc=yes
 
 if test "$have_clang" = yes; then
 
+	clang_minor=$( $CXX -dM -E -x c /dev/null | grep __clang_minor__ | sed 's/.*\([0-9][0-9]*\).*/\1/' )
+	clang_patch=$( $CXX -dM -E -x c /dev/null | grep __clang_patchlevel__ | sed 's/.*\([0-9][0-9]*\).*/\1/' )
+	clang_major=$( $CXX -dM -E -x c /dev/null | grep __clang_major__ | sed 's/.*\([0-9][0-9]*\).*/\1/' )
+
+	cxx_version="$clang_major.$clang_minor.$clang_patch"
+
 	is_xcode=$( $CXX -dM -E -x c /dev/null | grep __apple_build_version__ )
 
+  # Need at least version 8
 	if test -n "$is_xcode"; then
-		clang_minor=$( $CXX -dM -E -x c /dev/null | grep __clang_minor__ | sed 's/.*\([0-9][0-9]*\).*/\1/' )
-		clang_patch=$( $CXX -dM -E -x c /dev/null | grep __clang_patchlevel__ | sed 's/.*\([0-9][0-9]*\).*/\1/' )
-		clang_major=$( $CXX -dM -E -x c /dev/null | grep __clang_major__ | sed 's/.*\([0-9][0-9]*\).*/\1/' )
-
-		cxx_version="$clang_major.$clang_minor.$clang_patch"
 		cxx_name="XCode $cxx_version"
 
 		if test $clang_major -ge 8; then
@@ -401,35 +403,23 @@ if test "$have_clang" = yes; then
 			cxx_verc_fail=no
 		else
 			cxx_version="$cxx_version, bad"
-			cxx_verc_fail=bad
+			cxx_verc_fail=yes
 		fi
 	else
-		cxx_name=`( $cc -v ) 2>&1 | tail -n 1 | cut -d ' ' -f 1`
+    # Need at least version 3.5
-		cxx_version=$( $CXX -dM -E -x c /dev/null | grep __clang_version__ | sed 's/^.*[^0-9]\([0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\).*$/\1/g' 2>&1)
+		if [ $clang_major -ge 4 ] || [ $clang_major -eq 3 -a $clang_minor -ge 5 ]; then
-		if test "$?" -gt 0; then
+			cxx_version="$cxx_version, ok"
-			cxx_version="not found"
+			cxx_verc_fail=no
+		else
+			cxx_version="$cxx_version, bad"
+			cxx_verc_fail=yes
 		fi
-
-		case $cxx_version in
-			[3].[4-9]|[3].[4-9].[0-9]|[3].[4-9].[0-9][-.]*|[4-9].[0-9].[0-9])
-				_cxx_major=`echo $cxx_version | cut -d '.' -f 1`
-				_cxx_minor=`echo $cxx_version | cut -d '.' -f 2`
-				cxx_version="$cxx_version, ok"
-				cxx_verc_fail=no
-				;;
-			'not found')
-				cxx_verc_fail=yes
-				;;
-			*)
-				cxx_version="$cxx_version, bad"
-				cxx_verc_fail=yes
-				;;
-		esac
 	fi
 	CXXFLAGS="$CXXFLAGS"
 	_make_def_HAVE_GCC3='HAVE_GCC3 = 1'
 	add_line_to_config_mk 'CXX_UPDATE_DEP_FLAG = -MMD -MF "$(*D)/$(DEPDIR)/$(*F).d" -MQ "$@" -MP'
 	_make_def_HAVE_GCC='HAVE_GCC = 1'
+	_make_def_HAVE_CLANG='HAVE_CLANG = 1'
 	echo "$cxx_version"
 
 elif test "$have_gcc" = yes; then
@@ -505,7 +495,7 @@ fi
 if test "$cxx_verc_fail" = yes ; then
 	echo
 	echo "The version of your compiler is not supported at this time"
-	echo "Please ensure you are using GCC 4.7 / Clang 3.4 or above"
+	echo "Please ensure you are using GCC 5.0 / Clang 3.8 or above"
 	exit 1
 fi
 
@@ -826,6 +816,7 @@ PROFILE := $_build_profile
 
 $_make_def_HAVE_GCC
 $_make_def_HAVE_GCC3
+$_make_def_HAVE_CLANG
 
 INCLUDES += $INCLUDES
 OBJS += $OBJS

src/common/Base.hxx

@@ -98,6 +98,7 @@ class Base
 
     // Format specifiers to use for sprintf (eventually we may convert
     // to C++ streams
+    static ostringstream buf;
     static const char* const myLowerFmt[4];
     static const char* const myUpperFmt[4];
     static const char* const* myFmt;

src/common/SDL_lib.hxx

@@ -24,8 +24,10 @@
  */
 #ifdef __clang__
   #pragma clang diagnostic push
-  #pragma clang diagnostic ignored "-Wimplicit-fallthrough"
   #pragma clang diagnostic ignored "-Wdocumentation"
+  #pragma clang diagnostic ignored "-Wdocumentation-unknown-command"
+  #pragma clang diagnostic ignored "-Wimplicit-fallthrough"
+  #pragma clang diagnostic ignored "-Wreserved-id-macro"
   #include <SDL.h>
   #pragma clang diagnostic pop
 #else

src/common/StringParser.hxx

@@ -50,7 +50,7 @@ class StringParser
       @param str    The string to split
       @param maxlen The maximum length of string to generate
     */
-    StringParser(const string& str, uInt16 maxlen)
+    StringParser(const string& str, uInt32 maxlen)
     {
       istringstream buf(str);
       string line;

src/common/bspf.hxx

@@ -22,7 +22,7 @@
   This file defines various basic data types and preprocessor variables
   that need to be defined for different operating systems.
 
-  @author Bradford W. Mott
+  @author Bradford W. Mott and Stephen Anthony
 */
 
 #include <cstdint>
@@ -77,6 +77,7 @@ using std::memcpy;
 using IntArray = std::vector<Int32>;
 using BoolArray = std::vector<bool>;
 using ByteArray = std::vector<uInt8>;
+using ShortArray = std::vector<uInt16>;
 using StringList = std::vector<std::string>;
 using BytePtr = std::unique_ptr<uInt8[]>;
 
@@ -87,12 +88,14 @@ namespace BSPF
   // Defines to help with path handling
   #if defined(BSPF_UNIX) || defined(BSPF_MAC_OSX)
     static const string PATH_SEPARATOR = "/";
+    #define ATTRIBUTE_FMT_PRINTF __attribute__((__format__ (__printf__, 2, 0)))
   #elif defined(BSPF_WINDOWS)
     static const string PATH_SEPARATOR = "\\";
     #pragma warning (disable : 4146)  // unary minus operator applied to unsigned type
     #pragma warning(2:4264)  // no override available for virtual member function from base 'class'; function is hidden
     #pragma warning(2:4265)  // class has virtual functions, but destructor is not virtual
     #pragma warning(2:4266)  // no override available for virtual member function from base 'type'; function is hidden
+    #define ATTRIBUTE_FMT_PRINTF
   #else
     #error Update src/common/bspf.hxx for path separator
   #endif
@@ -164,13 +167,13 @@ namespace BSPF
 
   // Find location (if any) of the second string within the first,
   // starting from 'startpos' in the first string
-  inline size_t findIgnoreCase(const string& s1, const string& s2, int startpos = 0)
+  inline size_t findIgnoreCase(const string& s1, const string& s2, size_t startpos = 0)
   {
     auto pos = std::search(s1.cbegin()+startpos, s1.cend(),
       s2.cbegin(), s2.cend(), [](char ch1, char ch2) {
         return toupper(uInt8(ch1)) == toupper(uInt8(ch2));
       });
-    return pos == s1.cend() ? string::npos : size_t(pos - (s1.cbegin()+startpos));
+    return pos == s1.cend() ? string::npos : pos - (s1.cbegin()+startpos);
   }
 
   // Test whether the first string ends with the second one (case insensitive)

src/debugger/CartDebug.cxx

@@ -58,7 +58,7 @@ CartDebug::CartDebug(Debugger& dbg, Console& console, const OSystem& osystem)
   mySystemAddresses = LabelToAddr(sysCmp);
 
   // Add Zero-page RAM addresses
-  for(uInt32 i = 0x80; i <= 0xFF; ++i)
+  for(uInt16 i = 0x80; i <= 0xFF; ++i)
   {
     myState.rport.push_back(i);
     myState.wport.push_back(i);
@@ -75,14 +75,15 @@ CartDebug::CartDebug(Debugger& dbg, Console& console, const OSystem& osystem)
 
   BankInfo info;
   info.size = std::min(banksize, 4096u);
-  for(int i = 0; i < myConsole.cartridge().bankCount(); ++i)
+  for(uInt32 i = 0; i < myConsole.cartridge().bankCount(); ++i)
     myBankInfo.push_back(info);
 
   info.size = 128;  // ZP RAM
   myBankInfo.push_back(info);
 
   // We know the address for the startup bank right now
-  myBankInfo[myConsole.cartridge().startBank()].addressList.push_front(myDebugger.dpeek(0xfffc));
+  myBankInfo[myConsole.cartridge().startBank()].addressList.push_front(
+    myDebugger.dpeek(0xfffc));
   addLabel("Start", myDebugger.dpeek(0xfffc, DATA));
 
   // Add system equates
@@ -132,7 +133,7 @@ const DebuggerState& CartDebug::getState()
 {
   myState.ram.clear();
   for(uInt32 i = 0; i < myState.rport.size(); ++i)
-    myState.ram.push_back(peek(myState.rport[i]));
+    myState.ram.push_back(myDebugger.peek(myState.rport[i]));
 
   if(myDebugWidget)
     myState.bank = myDebugWidget->bankState();
@@ -145,7 +146,7 @@ void CartDebug::saveOldState()
 {
   myOldState.ram.clear();
   for(uInt32 i = 0; i < myOldState.rport.size(); ++i)
-    myOldState.ram.push_back(peek(myOldState.rport[i]));
+    myOldState.ram.push_back(myDebugger.peek(myOldState.rport[i]));
 
   if(myDebugWidget)
   {
@@ -1333,7 +1334,7 @@ void CartDebug::getBankDirectives(ostream& buf, BankInfo& info) const
   buf << "ORG " << Base::HEX4 << info.offset << endl;
 
   // Now consider each byte
-  uInt32 prev = info.offset, addr = prev + 1;
+  uInt16 prev = info.offset, addr = prev + 1;
   DisasmType prevType = disasmTypeAbsolute(mySystem.getAccessFlags(prev));
   for( ; addr < info.offset + info.size; ++addr)
   {

src/debugger/CartDebug.hxx

@@ -39,10 +39,10 @@ using CartMethod = int (CartDebug::*)();
 class CartState : public DebuggerState
 {
   public:
-    ByteArray ram;   // The actual data values
+    ByteArray ram;    // The actual data values
-    IntArray rport;  // Address for reading from RAM
+    ShortArray rport; // Address for reading from RAM
-    IntArray wport;  // Address for writing to RAM
+    ShortArray wport; // Address for writing to RAM
-    string bank;     // Current banking layout
+    string bank;      // Current banking layout
 };
 
 class CartDebug : public DebuggerSystem
@@ -113,13 +113,6 @@ class CartDebug : public DebuggerSystem
     CartDebugWidget* getDebugWidget() const { return myDebugWidget; }
     void setDebugWidget(CartDebugWidget* w) { myDebugWidget = w; }
 
-    // The following assume that the given addresses are using the
-    // correct read/write port ranges; no checking will be done to
-    // confirm this.
-    uInt8 peek(uInt16 addr)   { return mySystem.peek(addr); }
-    uInt16 dpeek(uInt16 addr) { return mySystem.peek(addr) | (mySystem.peek(addr+1) << 8); }
-    void poke(uInt16 addr, uInt8 value) { mySystem.poke(addr, value); }
-
     // Indicate that a read from write port has occurred at the specified
     // address.
     void triggerReadFromWritePort(uInt16 address);
@@ -295,7 +288,7 @@ class CartDebug : public DebuggerSystem
       uInt16 start;                // start of address space
       uInt16 end;                  // end of address space
       uInt16 offset;               // ORG value
-      uInt16 size;                 // size of a bank (in bytes)
+      uInt32 size;                 // size of a bank (in bytes)
       AddressList addressList;     // addresses which PC has hit
       DirectiveList directiveList; // overrides for automatic code determination
 

src/debugger/Debugger.hxx

@@ -169,9 +169,9 @@ class Debugger : public DialogContainer
     static uInt8 set_bit(uInt8 input, uInt8 bit, bool on)
     {
       if(on)
-        return input | (1 << bit);
+        return uInt8(input | (1 << bit));
       else
-        return input & ~(1 << bit);
+        return uInt8(input & ~(1 << bit));
     }
     static void set_bits(uInt8 reg, BoolArray& bits)
     {
@@ -193,7 +193,7 @@ class Debugger : public DialogContainer
       return result;
     }
 
-    /* Invert given input if it differs from its previous value */
+    /** Invert given input if it differs from its previous value */
     const string invIfChanged(int reg, int oldReg);
 
     /**
@@ -206,9 +206,25 @@ class Debugger : public DialogContainer
     */
     static Debugger& debugger() { return *myStaticDebugger; }
 
-    /* These are now exposed so Expressions can use them. */
+    /** Convenience methods to access peek/poke from System */
-    int peek(int addr, uInt8 flags = 0) { return mySystem.peek(addr, flags); }
+    uInt8 peek(uInt16 addr, uInt8 flags = 0) {
-    int dpeek(int addr, uInt8 flags = 0) { return mySystem.peek(addr, flags) | (mySystem.peek(addr+1, flags) << 8); }
+      return mySystem.peek(addr, flags);
+    }
+    uInt16 dpeek(uInt16 addr, uInt8 flags = 0) {
+      return uInt16(mySystem.peek(addr, flags) | (mySystem.peek(addr+1, flags) << 8));
+    }
+    void poke(uInt16 addr, uInt8 value, uInt8 flags = 0) {
+      mySystem.poke(addr, value, flags);
+    }
+
+    /** These are now exposed so Expressions can use them. */
+    int peekAsInt(int addr, uInt8 flags = 0) {
+      return mySystem.peek(uInt16(addr), flags);
+    }
+    int dpeekAsInt(int addr, uInt8 flags = 0) {
+      return mySystem.peek(uInt16(addr), flags) |
+             (mySystem.peek(uInt16(addr+1), flags) << 8);
+    }
     int getAccessFlags(uInt16 addr) const
       { return mySystem.getAccessFlags(addr); }
     void setAccessFlags(uInt16 addr, uInt8 flags)

src/debugger/DebuggerExpressions.hxx

@@ -337,7 +337,7 @@ class WordDerefExpression : public Expression
   public:
     WordDerefExpression(Expression* left) : Expression(left) { }
     Int32 evaluate() const override
-      { return Debugger::debugger().dpeek(myLHS->evaluate()); }
+      { return Debugger::debugger().dpeekAsInt(myLHS->evaluate()); }
 };
 
 #endif

src/debugger/gui/Cart0840Widget.cxx

@@ -36,7 +36,7 @@ Cartridge0840Widget::Cartridge0840Widget(
   for(uInt32 i = 0, offset = 0xFFC, spot = 0x800; i < 2;
       ++i, offset += 0x1000, spot += 0x40)
   {
-    uInt16 start = (cart.myImage[offset+1] << 8) | cart.myImage[offset];
+    uInt16 start = uInt16((cart.myImage[offset+1] << 8) | cart.myImage[offset]);
     start -= start % 0x1000;
     info << "Bank " << i << " @ $" << Common::Base::HEX4 << start << " - "
          << "$" << (start + 0xFFF) << " (hotspot = $" << spot << ")\n";

src/debugger/gui/PromptWidget.hxx

@@ -35,8 +35,8 @@ class PromptWidget : public Widget, public CommandSender
     virtual ~PromptWidget() = default;
 
   public:
-    int printf(const char* format, ...);
+    ATTRIBUTE_FMT_PRINTF int printf(const char* format, ...);
-    int vprintf(const char* format, va_list argptr);
+    ATTRIBUTE_FMT_PRINTF int vprintf(const char* format, va_list argptr);
     void print(const string& str);
     void printPrompt();
     bool saveBuffer(const FilesystemNode& file);

src/debugger/gui/RiotRamWidget.cxx

@@ -32,14 +32,14 @@ RiotRamWidget::RiotRamWidget(GuiObject* boss, const GUI::Font& lfont,
 uInt8 RiotRamWidget::getValue(int addr) const
 {
   const CartState& state = static_cast<const CartState&>(myDbg.getState());
-  return myDbg.peek(state.rport[addr]);
+  return instance().debugger().peek(state.rport[addr]);
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 void RiotRamWidget::setValue(int addr, uInt8 value)
 {
   const CartState& state = static_cast<const CartState&>(myDbg.getState());
-  myDbg.poke(state.wport[addr], value);
+  instance().debugger().poke(state.wport[addr], value);
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

src/emucore/Thumbulator.hxx

@@ -27,6 +27,13 @@
 #ifndef THUMBULATOR_HXX
 #define THUMBULATOR_HXX
 
+// FIXME - This code has many instances of shifting into signed integers
+//         Perhaps the int's should be changed to uInt32
+#ifdef __clang__
+  #pragma clang diagnostic push
+  #pragma clang diagnostic ignored "-Wshift-sign-overflow"
+#endif
+
 #include "bspf.hxx"
 #include "Cart.hxx"
 #include "Console.hxx"

src/emucore/tia/FrameManager.cxx

@@ -237,7 +237,7 @@ void FrameManager::setState(FrameManager::State state)
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 void FrameManager::finalizeFrame()
 {
-  if (myCurrentFrameTotalLines != (uInt32)myStableFrameLines) {
+  if (myCurrentFrameTotalLines != uInt32(myStableFrameLines)) {
     if (myCurrentFrameTotalLines == myCurrentFrameFinalLines) {
 
       if (++myStableFrameHeightCountdown >= Metrics::framesForStableHeight) {
@@ -273,7 +273,7 @@ void FrameManager::finalizeFrame()
 void FrameManager::handleJitter(Int32 scanlineDifference)
 {
   if (
-    (uInt32)abs(scanlineDifference) < Metrics::minDeltaForJitter ||
+    uInt32(abs(scanlineDifference)) < Metrics::minDeltaForJitter ||
     !myJitterEnabled ||
     myTotalFrames < Metrics::initialGarbageFrames
   ) return;

src/emucore/tia/PaddleReader.cxx

@@ -69,7 +69,7 @@ void PaddleReader::update(double value, double timestamp, ConsoleTiming consoleT
     setConsoleTiming(consoleTiming);
   }
 
-  if (value != myValue) {
+  if (value != myValue) {  // FIXME - warning on 'no-float-equal'
     myValue = value;
 
     if (myValue < 0) {

src/yacc/YaccParser.cxx

@@ -32,6 +32,7 @@ void yyerror(const char* e);
 
 #ifdef __clang__
   #pragma clang diagnostic push
+  #pragma clang diagnostic ignored "-Wold-style-cast"
   #pragma clang diagnostic ignored "-Wimplicit-fallthrough"
   #pragma clang diagnostic ignored "-Wmissing-variable-declarations"
   #include "y.tab.c"