Static Analysis #202

toxieainc · 2024-10-02T18:58:21Z

Addresses mostly harmless things found by static analysis while trying to nail down #178

trzy

Mostly nitpicks, a few questions about code being deleted.

trzy · 2024-10-02T21:21:14Z

Src/GameLoader.cpp

@@ -587,7 +587,7 @@ void GameLoader::IdentifyGamesInZipArchive(
      Region::ptr_t region = v2.second;
      if (!region->required)
        continue;
-      for (auto file: region->files)
+      for (const auto& file: region->files)


Can you make this "const auto &file". I prefer that style (and it's consistent with elsewhere in the code that I've written).

trzy · 2024-10-02T21:21:22Z

Src/GameLoader.cpp

@@ -757,7 +757,7 @@ bool GameLoader::ComputeRegionSize(uint32_t *region_size, const GameLoader::Regi
  // use maximum end_addr = offset + stride * (num_chunks - 1) + chunk_size.
  std::vector<uint32_t> end_addr;
  bool error = false;
-  for (auto file: region->files)
+  for (const auto& file: region->files)


trzy · 2024-10-02T21:23:47Z

Src/Graphics/Legacy3D/TextureRefs.cpp

@@ -261,8 +261,7 @@ void CTextureRefs::DeleteAllHashEntries()
 	for (unsigned i = 0; i < m_hashCapacity; i++)
 	{
 		HashEntry *entry = m_hashEntries[i];
-		if (entry)


Are you sure this check isn't needed here?

Yes, nowadays all compilers respect the c++ standard, that says 'In all cases, if ptr is a null pointer, the standard library deallocation functions do nothing.'

trzy · 2024-10-02T21:24:07Z

Src/Graphics/Render2D.cpp

@@ -411,7 +411,7 @@ void CRender2D::AttachVRAM(const uint8_t* vramPtr)
 {
 }

-void CRender2D::AttachDrawBuffers(std::shared_ptr<TileGenBuffer> bottom, std::shared_ptr<TileGenBuffer> top)
+void CRender2D::AttachDrawBuffers(const std::shared_ptr<TileGenBuffer>& bottom, const std::shared_ptr<TileGenBuffer>& top)


Nit: Move & to align with left of parameter name.

This change really isn't needed lol

I guess @dukeeeey is right, this is maybe a bit stupid

trzy · 2024-10-02T21:25:45Z

Src/Graphics/Render2D.h

 	*/
-	void AttachDrawBuffers(std::shared_ptr<TileGenBuffer> bottom, std::shared_ptr<TileGenBuffer> top);
+	void AttachDrawBuffers(const std::shared_ptr<TileGenBuffer>& bottom, const std::shared_ptr<TileGenBuffer>& top);


Nit: &bottom, &top

trzy · 2024-10-02T21:29:40Z

Src/OSD/Logger.h

@@ -183,13 +183,13 @@ class CFileLogger: public CLogger
  void DebugLog(const char *fmt, va_list vl);
  void InfoLog(const char *fmt, va_list vl);
  void ErrorLog(const char *fmt, va_list vl);
-	CFileLogger(LogLevel level, std::vector<std::string> filenames);
-  CFileLogger(LogLevel level, std::vector<std::string> filenames, std::vector<FILE *> systemFiles);
+  CFileLogger(LogLevel level, const std::vector<std::string>& filenames);


& alignment

trzy · 2024-10-02T21:29:48Z

Src/OSD/Logger.h

-	CFileLogger(LogLevel level, std::vector<std::string> filenames);
-  CFileLogger(LogLevel level, std::vector<std::string> filenames, std::vector<FILE *> systemFiles);
+  CFileLogger(LogLevel level, const std::vector<std::string>& filenames);
+  CFileLogger(LogLevel level, const std::vector<std::string>& filenames, const std::vector<FILE *>& systemFiles);


& alignment

trzy · 2024-10-02T21:31:32Z

Src/Sound/SCSPDSP.cpp

@@ -479,13 +479,14 @@ void SCSPDSP_Start(_SCSPDSP *DSP)
 	}
 */

-	for(int t=0;t<0x10000;++t)
+/*	for(int t=0;t<0x10000;++t)


Why is this loop being commented out?

It is simply debug code it seems, as it has no effect in practice (only if one would set a breakpoint in there or the like).

trzy · 2024-10-02T21:32:30Z

Src/Util/GenericValue.h

@@ -29,19 +29,19 @@ namespace Util
    template <typename T>
    struct IntegerEncodableAsHex
    {
-      static const bool value = std::is_integral<T>::value && sizeof(T) >= 2 && sizeof(T) <= 8;


It's been a long time since I've looked at this template stuff but I thought these ::value and ::type members were required?

The analyzer recommended to modernize it, so it should be equivalent.

trzy · 2024-10-02T21:32:40Z

Src/Util/NewConfig.cpp

@@ -238,7 +238,7 @@ namespace Util
      if (Exists())
        m_value->Serialize(os);
      *os << "\"  children={";
-      for (auto v: m_children)
+      for (const auto& v: m_children)


const auto &v:

toxieainc · 2024-10-03T06:44:31Z

All should be addressed, thanks!

toxieainc · 2024-10-04T13:07:43Z

@trzy Is this okay like this?

trzy · 2024-10-05T00:50:48Z

Looks good. Thank you!

toxieainc · 2024-10-05T08:38:45Z

Another question regarding constructors, do you prefer the 'classic' initialization a la

Bla::Bla() : m_member1(0), m_member2(nullptr), ... ...
{
   memset(...);
   ...
}

or via default member initialization directly in the class/member declaration

class Bla
{
   uint m_member1 = 0;
   void* m_member2 = nullptr;
   ...
}

Nowadays i personally prefer the latter as then its easier to spot if something has been forgotten, and also the order is clear, if there are dependencies.

dukeeeey · 2024-10-05T08:49:24Z

I normally use the latter for structs especially if they have no constructor at all, and the former for classes. If all elements are set to zero or null it just kind of needlessly fattens the class header. But this is just personal preference, I don't think there is a right and wrong answer for this.

toxieainc · 2024-10-05T17:17:08Z

Okay, as i'm currently preparing a PR for more class inits, what to do in case if there is currently a mixture? So some members use a), some b).

dukeeeey · 2024-10-05T17:29:13Z

If it has a mixture I mean you can refactor if you want but there isn't a strong need for it. As long as the classes are initialised to a known state.

Some of the structs used for rendering its preferable for performance that they not be initialised. All members will be written to anyway.

toxieainc · 2024-10-05T17:57:19Z

Absolutely, the 'small' critical ones i will not touch..

address mostly harmless things found by static analysis

46453ed

trzy reviewed Oct 2, 2024

View reviewed changes

address reviews

d9e1cc0

some more from the review

3f6fe29

trzy merged commit f573681 into trzy:master Oct 5, 2024

toxieainc deleted the analysis_fixes branch October 5, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static Analysis #202

Static Analysis #202

toxieainc commented Oct 2, 2024

trzy left a comment

trzy Oct 2, 2024

trzy Oct 2, 2024

trzy Oct 2, 2024

toxieainc Oct 3, 2024

trzy Oct 2, 2024

dukeeeey Oct 2, 2024

toxieainc Oct 3, 2024

trzy Oct 2, 2024

trzy Oct 2, 2024

trzy Oct 2, 2024

trzy Oct 2, 2024

toxieainc Oct 3, 2024

trzy Oct 2, 2024

toxieainc Oct 3, 2024

trzy Oct 2, 2024

toxieainc commented Oct 3, 2024

toxieainc commented Oct 4, 2024

trzy commented Oct 5, 2024

toxieainc commented Oct 5, 2024

dukeeeey commented Oct 5, 2024

toxieainc commented Oct 5, 2024

dukeeeey commented Oct 5, 2024

toxieainc commented Oct 5, 2024

Static Analysis #202

Static Analysis #202

Conversation

toxieainc commented Oct 2, 2024

trzy 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toxieainc commented Oct 3, 2024

toxieainc commented Oct 4, 2024

trzy commented Oct 5, 2024

toxieainc commented Oct 5, 2024

dukeeeey commented Oct 5, 2024

toxieainc commented Oct 5, 2024

dukeeeey commented Oct 5, 2024

toxieainc commented Oct 5, 2024