Skip to content
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++17 introduction and minor clean-ups #453

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

markuspg
Copy link
Contributor

C++17 is default with GCC 11 (released in April 2021) and LLVM 16 (released in March 2023). It's fully covered since GCC 8 (released in May 2018) and LLVM 5 (released in September 2017).

Apart of this change there are a few minor adjustments.

This adds some nifty features and should be well supported by now (at
least it should work with Ubuntu 20.04).

Signed-off-by: markuspg <[email protected]>
@@ -1,3 +1,34 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not much to see here (and in bmap.h), just some missing license headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is okay to add BSD license headers, just remove copy write line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped the Copyright ... lines.

@@ -62,7 +62,7 @@ static map<string, shared_ptr<FileBuffer>> g_filebuffer_map;
static mutex g_mutex_map;
static bool g_small_memory = true;

#define MAGIC_PATH '>'
static constexpr char MAGIC_PATH = '>';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To my knowledge #defines are generally being frowned upon today. I took the switch to C++17 as an opportunity to replace all of these which defined constants by constexpr "variables".

@@ -139,7 +139,7 @@ class FSBasic

virtual int Decompress(const string& /*backfifle*/, shared_ptr<FileBuffer> /*outp*/) { return 0; };
virtual bool seekable(const string& /*backfile*/) { return false; }
virtual std::shared_ptr<FragmentBlock> ScanCompressblock(const string& /*backfile*/, size_t& /*input_offset*/, size_t& /*output_offset*/) { return NULL; };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few NULL constants were around still. I replaced these by nullptr.

libuuu/bmap.cpp Outdated
@@ -1,3 +1,34 @@
/*
* Copyright 2025 NXP.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Done't claim nxp copyright for this file, which is added by no-nxp contribtor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped

Copy link
Contributor

@nxpfrankli nxpfrankli left a comment

Choose a reason for hiding this comment

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

Can you descript which C++ features used and have to bump to 17? Also need fix vs build issue
Build with VS Studio / build (Debug, x86) (pull_request)

@markuspg
Copy link
Contributor Author

In this pull request it's the "inline variables" from C++17 which I utilized. I thought some of the new containers would be practical too (e.g. to replace the union in the uuu_notify struct, although on a second look there don't seem to be much more opportunities to put them to use).

Copy link
Contributor

@nxpfrankli nxpfrankli left a comment

Choose a reason for hiding this comment

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

Can you descript which C++ features cause have to bump to c++ 17. And
VS2017 build failure, D:\a\mfgtools\mfgtools\libuuu\buffer.h(65,22): error C7525: inline variables require at least '/std:c++17' [D:\a\mfgtools\mfgtools\msvc\libuuu.vcxproj]
D:\a\mfgtools\mfgtools\libuuu\buffer.h(66,22): error C7525: inline variables require at least '/std:c++17' [D:\a\mfgtools\mfgtools\msvc\libuuu.vcxproj]
D:\a\mfgtools\mfgtools\libuuu\buffer.h(67,22): error C7525: inline variables require at least '/std:c++17' [D:\a\mfgtools\mfgtools\msvc\libuuu.vcxproj]
D:\a\mfgtools\mfgtools\libuuu\buffer.h(68,22): error C7525: inline variables require at least '/std:c++17' [D:\a\mfgtools\mfgtools\msvc\libuuu.vcxproj]
D:\a\mfgtools\mfgtools\libuuu\buffer.h(69,22): error C7525: inline variables require at least '/std:c++17' [D:\a\mfgtools\mfgtools\msvc\libuuu.vcxproj]
D:\a\mfgtools\mfgtools\libuuu\buffer.h(71,22): error C7525: inline variables require at least '/std:c++17' [D:\a\mfgtools\mfgtools\msvc\libuuu.vcxproj]
D:\a\mfgtools\mfgtools\libuuu\buffer.h(72,22): error C7525: inline variables require at least '/std:c++17' [D:\a\mfgtools\mfgtools\msvc\libuuu.vcxproj]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants