Uploaded image for project: 'Qt Creator'
  1. Qt Creator
  2. QTCREATORBUG-13155

Implement correct auto-indent behaviour for break in switch statement and some other small questions

    XMLWordPrintable

Details

    • b72a9dd2391680b7a9ed7c82c1cfefc7cef687e8

    Description

      I use Qt Creator as my main C++ IDE and really appreciate it. So I want to contribute a little. To begin with I want to fix a small issue, which annoys me. Generally I prefer this style of "switch/case" intendation:

      switch (...) {
         case 1:
            ...
         break;
         case 2:
            ...
         break;
      }
      

      so you are always able to see a missed "break". I set up my code style preferences in Qt Creator as in picture below and preview looks as I want:

      When you are typing a code with this settings and write "case something:", after you type a colon, code line automatically get a proper indent, but when you write "break;", it doesn't happen and you are forced to indent it manually (e.g. by pressing Ctrl+I shortcut). I would like to fix this and make the same behavior as in "case" scenario. To do it right, I want to ask a few questions (if it's not a right place, feel free to redirect me):

      1) Which behavior is better: change indent when a semicolon after break was typed or after Enter was pressed? Imo, first is better.

      2) I tried to implement first approach and after some source exploration it was easy, i just add a semicolon into switch in CppQtStyleIndenter::isElectricCharacter from cppqtstyleindenter.cpp:

      bool CppQtStyleIndenter::isElectricCharacter(const QChar &ch) const
      {
          switch (ch.toLatin1()) {
          case '{':
          case '}':
          case ':':
          case '#':
          case '<':
          case '>':
      

      +   case ';':

              return true;
          }
          return false;
      }
      

      and a check for break in isElectricInLine from the same file:

      static bool isElectricInLine(const QChar ch, const QString &text)
      {
          switch (ch.toLatin1()) {
      

      +   case ';':
      +       return text.contains(QLatin1String("break"));
      

          case ':':
              // switch cases and access declarations should be reindented
              if (text.contains(QLatin1String("case"))
                      || text.contains(QLatin1String("default"))
                      || text.contains(QLatin1String("public"))
                      || text.contains(QLatin1String("private"))
                      || text.contains(QLatin1String("protected"))
                      || text.contains(QLatin1String("signals"))
                      || text.contains(QLatin1String("Q_SIGNALS"))) {
                  return true;
              }
      

      It works, but is it consider OK and does not make any side effects? Actually isElectricInLine function doesn't smell good for me, it just checks for a substring (so for strings like "any_case_of" it will return true), but because it doesn't take part in indent calculation (only in check can reindent be applied) and an additional check in function, which make reindention (it's CppQtStyleIndenter::indentBlock from the same file as previous functions), this "dirty checking" seems to work

      3) Should i write some auto test (I don't learn how to do it yet) for check this behavior and can such a test be maked?

      4) When exploring a source, I met this code in QtStyleCodeFormatter::adjustIndent from cppcodeformatter.cpp:

          case T_BREAK:
          case T_CONTINUE:
          case T_RETURN:
              if (topState.type == case_cont) {
                  *indentDepth = topState.savedIndentDepth;
                  if (m_styleSettings.indentControlFlowRelativeToSwitchLabels)
                      *indentDepth += m_tabSettings.m_indentSize;
              }
          }
      

      As you can see here, for intendation purposes inside switch statements continue and return are considered the same as break (and they actually are, you can easily test it via Ctrl+I shortcut with the same indentation settings as in screenshot above). Is it intended to be like that, because in "Edit Code Style" preference's text says only about break: "break" statement relative to "case" or "default"? As for me, I don't like this behavior, but if it's intended, checks for continue and return should be added to isElectricInLine in addition to break's one.

      5) In isElectricCharacter and isElectricInLine implementations you can see, that '<' and '>' characters consider electric (characters that cause current line to reindent). If I understand properly, they should be applied in multi-line template declarations for padding correction. There is also some code for padding calculation for templates, but despite trying with different settings I couldn't call reindent behaviour in this case (Ctrl+I also didn't work). Do I miss something?

      6) Also I found 4 code fragments, that can be refactored (all fragments from cppcodeformatter.cpp):

                  switch (kind) {
                  case T_RBRACE:      leave(); continue;
                  case T_DEFAULT:
                  case T_CASE:        leave(); continue;
                  } break;
      

      starts at line 461. Upper "leave(); continue;" can be removed.

              for (int i = 0; state(i).type != topmost_intro; ++i) {
                  const int type = state(i).type;
                  if (type == switch_statement) {
                      *indentDepth = state(i).savedIndentDepth;
                      if (m_styleSettings.indentSwitchLabels)
                          *indentDepth += m_tabSettings.m_indentSize;
                      break;
                  } else if (type == case_cont) {
                      *indentDepth = state(i).savedIndentDepth;
                      break;
                  } else if (type == topmost_intro) {
                      break;
                  }
              }
      

      starts at line 1593. I think break; in else if (type == topmost_intro) is never executed, because of for-loop condition.

      for (; m_tokenIndex < m_tokens.size(); ) {
      

      on line 83. Shouldn't it be changed to while? And the last one, which already was showed above:

          case T_IDENTIFIER:
              if (topState.type == substatement
                      || topState.type == substatement_open
                      || topState.type == case_cont
                      || topState.type == block_open
                      || topState.type == defun_open) {
                  if (tokens.size() > 1 && tokens.at(1).kind() == T_COLON) // label?
                      *indentDepth = 0;
              }
              break;
          case T_BREAK:
          case T_CONTINUE:
          case T_RETURN:
              if (topState.type == case_cont) {
                  *indentDepth = topState.savedIndentDepth;
                  if (m_styleSettings.indentControlFlowRelativeToSwitchLabels)
                      *indentDepth += m_tabSettings.m_indentSize;
              }
          }
      

      starts at line 1652. The last closing brace is the end of very long (about 200 lines) switch. Should we insert a break; before this brace for consistency with other case statements (the case before this was included for clearness) in this switch? Can I edit all this fragments and should I do it with different commit or with the same, thats change indent behaviour (tbh I'm not very familiar with git workflow)? Thank you for reading and sorry for length and my bad English, I tried my best.

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          People

            kosjar Nikolai Kosjar
            lemelisk lemelisk
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes