Subset of clang-format changes. by ahmedcharles · Pull Request #965 · Mudlet/Mudlet

ahmedcharles · 2017-04-26T09:23:38Z

No description provided.

SlySven

I had "gone off the boil" before I got to the end there... so if there are any other issues then they slip by this time. OTOH If there are any issues then because this is just formatting they are not going to be made better or worse by this PR anyhow! 😀

SlySven · 2017-04-26T17:01:09Z

src/Host.cpp

        mInsertedMissingLF = true;
-        if( (cmd == "") && ( mUSE_IRE_DRIVER_BUGFIX ) && ( ! mUSE_FORCE_LF_AFTER_PROMPT ) )
-        {
+        if ((cmd == "") && (mUSE_IRE_DRIVER_BUGFIX) && (!mUSE_FORCE_LF_AFTER_PROMPT)) {


I'd prefer cmd.isEmpty() to avoid a raw string literal. 😁

SlySven · 2017-04-26T17:02:22Z

src/Host.cpp

-            sendRaw( "\n" );//NOTE: damit leerprompt moeglich sind
+    QStringList commandList = cmd.split(QString(mCommandSeparator), QString::SkipEmptyParts);
+    if (!dontExpandAliases) {
+        if (commandList.size() == 0) {


commandList.isEmpty() is more human readable than commandList.size() == 0 IMVHO...

SlySven · 2017-04-26T17:05:22Z

src/Host.cpp

+            continue;
+        }
        QString command = commandList[i];
        command.replace("\n", "");


Isn't this the same as command.remove("\n") - or even better command.remove(QChar::LineFeed)?

SlySven · 2017-04-26T17:11:29Z

src/Host.cpp

+        }
        QString command = commandList[i];
        command.replace("\n", "");
        mReplacementCommand = "";


As (void) Host::setReplacementCommand(const QString &) is not used, Host::replacementCommand never gets set to anything - so the fact that this clears it and dontExpandAliases is never set I think there is some cruft to clear out in this region.

SlySven · 2017-04-26T17:13:56Z

src/Host.cpp

 }

-void Host::raiseEvent( const TEvent & pE )
+void Host::raiseEvent(const TEvent& pE)


I thought I had been through and replaced pE with event as this is NOT a pointer (any more?) 😖

Ah, turns out that was #934 which I've had to hold to split clean up - so will need to cherry-pick and base on code post this one anyhow...!

SlySven · 2017-04-26T17:50:52Z

src/TAlias.cpp

-        {
-            qDebug()<<"CRITICAL ERROR: SHOULD NOT HAPPEN->pcre_info() got wrong num of cap groups ovector only has room for %d captured substrings\n";
+        if (rc == 0) {
+            qDebug() << "CRITICAL ERROR: SHOULD NOT HAPPEN->pcre_info() got wrong num of cap groups ovector only has room for %d captured substrings\n";


🪲 same bug as above - but this time I think the constant is 30 (see the pcre_exec(...) call).

SlySven · 2017-04-26T17:55:04Z

src/TArea.cpp

@@ -183,75 +171,75 @@ void TArea::determineAreaExitsOfRoom( int id )
    // The second term in the ifs below looks for exit room id in TArea
    // instance's own list of rooms which will fail (with a -1 if it is NOT in


Oops 😊 - I think I forgot to update the comment when I moved to a QSet<int> TArea::rooms data structure - the reference to a -1 result is no longer relevant!

SlySven · 2017-04-26T18:06:38Z

src/THighlighter.cpp


-    quotationFormat.setForeground( Qt::darkGreen );
-    rule.pattern = QRegExp( "\"[^\"]*\"" );
+    quotationFormat.setForeground(Qt::darkGreen);


🤔 That reminds me, quotation formatting breaks for escaped quotes in the Lua Script editor, i.e. a literal \" is actually treated like a plain " when one wants to embed " in a string to display to the user...

SlySven · 2017-04-26T18:11:03Z

src/dlgColorTrigger.cpp

-: QDialog( pF )
-, mpTrigger( pT )
-, mMode( mode )
+dlgColorTrigger::dlgColorTrigger(QWidget* pF, TTrigger* pT, int mode) : QDialog(pF), mpTrigger(pT), mMode(mode)


Sorry to pick at something that must be getting a bit scabbed by now but didn't we find a way/want to have the initialisations one per line?

SlySven · 2017-04-26T18:13:59Z

src/dlgColorTrigger.cpp

@@ -404,16 +387,15 @@ void dlgColorTrigger::setColorLightMagenta()

 void dlgColorTrigger::setColorWhite()


🤔 IIRC if we could set up a QSignalMappper we could considerably cut down on the "copy and modify" blocks of largely repetitive code here.

vadi2 · 2017-04-26T18:22:09Z

I am not sure why are you doing a full review of code for a PR that is just about changing the formatting of it... I mean the comments given aren't in scope of the PR and unless someone goes through and addresses them, they'll be lost?

vadi2 · 2017-04-26T18:25:34Z

They're very good comments, but addressing them would not be in scope of this PR, so what do we do with them?

SlySven · 2017-04-26T18:27:05Z

*thinks* Of course!

That was a stupid thing for me to waste time doing 😒

vadi2 · 2017-04-26T18:43:59Z

Well, I'll work on putting them in for you, don't want any time to be wasted. It just wasn't the right place to do such a thorough review on,

Ones I felt comfortable with anyhow

vadi2 · 2017-04-26T19:11:45Z

#968

vadi2 · 2017-04-26T19:14:10Z

src/TArea.cpp

        QPair<QString, int> exitData;
        exitData.second = itAreaExit.value().first;
-        switch( itAreaExit.value().second ) {
-            case DIR_NORTH:     exitData.first = QString("north");         break;


Would like to keep this - it's far more readable than the exploded variant

Ones I felt comfortable with anyhow

Subset of clang-format changes.

566b775

ahmedcharles added the 1 - Ready label Apr 26, 2017

ahmedcharles requested review from SlySven and vadi2 April 26, 2017 09:23

SlySven reviewed Apr 26, 2017

View reviewed changes

vadi2 added a commit to vadi2/Mudlet that referenced this pull request Apr 26, 2017

Implemented the improvements from Mudlet#965 review

5736a51

Ones I felt comfortable with anyhow

vadi2 requested changes Apr 26, 2017

View reviewed changes

fix

9ec0e73

vadi2 approved these changes Apr 27, 2017

View reviewed changes

ahmedcharles merged commit a28a173 into Mudlet:development Apr 27, 2017

ahmedcharles deleted the formatting branch April 27, 2017 10:55

vadi2 added a commit that referenced this pull request Apr 28, 2017

Implemented the improvements from #965 review (#968)

ab20b08

Ones I felt comfortable with anyhow

		@@ -183,75 +171,75 @@ void TArea::determineAreaExitsOfRoom( int id )
		// The second term in the ifs below looks for exit room id in TArea
		// instance's own list of rooms which will fail (with a -1 if it is NOT in

		@@ -404,16 +387,15 @@ void dlgColorTrigger::setColorLightMagenta()

		void dlgColorTrigger::setColorWhite()

Uh oh!

Conversation

ahmedcharles commented Apr 26, 2017

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Apr 26, 2017

Uh oh!

vadi2 commented Apr 26, 2017

Uh oh!

SlySven commented Apr 26, 2017

Uh oh!

vadi2 commented Apr 26, 2017

Uh oh!

vadi2 commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants