Skip to content

/ scummvm

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

BASE: Code formatting cleanup pass of base #984

Closed
wants to merge 1 commit into from
Closed

BASE: Code formatting cleanup pass of base #984

wants to merge 1 commit into from

Conversation

@bit-hack
Copy link

bit-hack commented Aug 8, 2017 •
edited

This is a non-functional change, that cleans up some of the source formatting in base.
Please let me know if you spot anything that is not in keeping with established ScummVM format.

Also if anyone knows how to get github to display 4 space tabs, please let me know :)
@bit-hack
BASE: Code formatting cleanup pass of base
Loading status checks…
573be70
@sev-
sev- requested changes Sep 3, 2017
View changes
Copy link
Member

sev- left a comment

While this PR has number of indeed good changes, it still has few questionable ones or those which are matter of taste.

I marked those, but taking into account number and diversity of touched files, I'm thinking about redoing it manually, unless those all are addressed.
base/commandLine.cpp
@@ -54,8 +54,7 @@ static const char USAGE_STRING[] =
"%s: %s\n"
"Usage: %s [OPTIONS]... [GAME]\n"
"\n"
"Try '%s --help' for more options.\n"
;

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

This was done on purpose.
base/commandLine.cpp
@@ -167,8 +166,7 @@ static const char HELP_STRING[] =
#endif
"\n"
"The meaning of boolean long options can be inverted by prefixing them with\n"
"\"no-\", e.g. \"--no-aspect-ratio\".\n"
;
"\"no-\", e.g. \"--no-aspect-ratio\".\n";

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

Same here. The purpose is to minimize diffs on changes.
base/main.cpp
@@ -153,24 +153,24 @@ static Common::Error runGame(const EnginePlugin *plugin, OSystem &system, const
// needed because otherwise the g_system->getSupportedFormats might return
// bad values.
g_system->beginGFXTransaction();
g_system->setGraphicsMode(ConfMan.get("gfx_mode").c_str());

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

This is done on purpose, to visually set up code which is within the transaction.
base/main.cpp
system.beginGFXTransaction();
// Set the user specified graphics mode (if any).
system.setGraphicsMode(ConfMan.get("gfx_mode").c_str());
// Set the user specified graphics mode (if any).

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

Same here, these are inside a code block.
base/main.cpp
@@ -551,17 +553,17 @@ extern "C" int scummvm_main(int argc, const char * const argv[]) {
#ifndef FORCE_RTL
if (result.getCode() == Common::kNoError && !g_system->getEventManager()->shouldRTL())
break;
#endif
#endif

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

Now, the #endif does not match the indentation of the #ifndef. Thus, either have both unindented, or do not make this change at all.
base/main.cpp
@@ -589,7 +591,8 @@ extern "C" int scummvm_main(int argc, const char * const argv[]) {
ConfMan.setActiveDomain("");
}

PluginManager::instance().loadAllPlugins(); // only for cached manager
// only for cached manager

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

Why this change?
base/plugins.cpp
extern PluginType g_##ID##_type; \
extern PluginObject *g_##ID##_getObject(); \
pl.push_back(new StaticPlugin(g_##ID##_getObject(), g_##ID##_type));
#define LINK_PLUGIN(ID) \

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

And this badly breaks visuals of the code.
base/plugins.h
// Plugin linking

#define STATIC_PLUGIN 1
#define DYNAMIC_PLUGIN 2

#define PLUGIN_ENABLED_STATIC(ID) \
(ENABLE_##ID && !PLUGIN_ENABLED_DYNAMIC(ID))
#define PLUGIN_ENABLED_STATIC(ID) (ENABLE_##ID && !PLUGIN_ENABLED_DYNAMIC(ID))

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

And now it is harder to read...
base/plugins.h
@@ -325,13 +312,16 @@ class PluginManager {
public:
virtual ~PluginManager();

static void destroy() { delete _instance; _instance = 0; }

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

It is interesting like you split this oneliner while adding few right above.
base/version.h
@@ -23,10 +23,10 @@
#ifndef BASE_VERSION_H
#define BASE_VERSION_H

extern const char *gScummVMVersion; // e.g. "0.4.1"

This comment has been minimized.

Sign in to view
Copy link
@sev-

sev- Sep 3, 2017

Member

This all was tab-aligned...
@csnover
Copy link
Member

csnover commented Oct 7, 2017

@8BitPimp are you planning on continuing to work on this patch?
@bit-hack
Copy link
Author

bit-hack commented Oct 12, 2017

@csnover Yes, I can redo these changes to address the issues raised by @sev- :)
Thanks for taking a look at the patch.
@sev-
Copy link
Member

sev- commented Jan 31, 2018

@8BitPimp Any update?
@sev-
Copy link
Member

sev- commented Mar 12, 2018

Still no updates?
@bonki
Copy link
Member

bonki commented Apr 23, 2018

@8BitPimp Are you planning on updating this PR?
@bonki
Copy link
Member

bonki commented Apr 24, 2018

Since there hasn't been any update for months I'm closing this for now, but feel free to reopen if you want to work on this PR again.
@bonki bonki closed this Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sev- sev-

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@bit-hack @csnover @sev- @bonki
You can’t perform that action at this time.