vim-jp / issues

有志で既知のバグや要望を検討・管理し、オフィシャルへの還元をしていきます。
https://vim-jp.org/
342 stars 11 forks source link

cinoptions=+ がnamespace 内を関数と認識する #140

Open thinca opened 12 years ago

thinca commented 12 years ago

91 から引き継ぎです。

関数内の継続行をインデントする :help cino-+ が、namespace {} 内を関数と認識してしまっているようです。 これにより、namespace 内に書いた テンプレート関数やテンプレートクラスがインデントされてしまいます。


// namespace 外
template <class T>
void func() {}
template <class T>
class Class {};

namespace {
// namespace 内
template <class T>
    void func() {}
template <class T>
    class Class {};
}
koron commented 12 years ago

http://vim-jp.org/vimdoc-ja/indent.html#cino-+

thinca commented 12 years ago

91 を見てもらうとわかると思いますが、cinoptions=N-s (namespace 内でインデントしない) の状態でも、namespace 内を関数内と誤認識して行継続のインデントがされてしまうようです。

mattn commented 12 years ago

単にブラケットがあったらインデントしてそう

h-east commented 12 years ago

誰かが調査する時の情報として貼っておきます。

該当関数: get_c_indent() [misc1.c : 6187行辺り] cinoptions=+NのN値を保持しているローカル変数: ind_continuation 継続行のインデント量を保持しているローカル変数: cont_amount ←行毎に変化しうる

関係ありそうな変数、関数 ourscope lookfor_cpp_namespace cin_is_cpp_namespace()

mattn commented 12 years ago

いまこの部分のソース見て昼飯をもどしそうになった。

h-east commented 12 years ago

Welcome to wonderland. :-) とりあえず 7057行辺りの判定はまったく意味が無いw

            else if (start_brace == BRACE_AT_START &&
                    lookfor_cpp_namespace)        /* '{' is at start */
            {

                lookfor_cpp_namespace = TRUE;
            }
            else
mattn commented 12 years ago

正しいかどうかそっちのけでやってみた。


diff -r 484291e1a8da src/misc1.c
--- a/src/misc1.c   Mon Jan 23 20:48:40 2012 +0100
+++ b/src/misc1.c   Wed Jan 25 14:18:04 2012 +0900
@@ -7068,7 +7068,10 @@

            l = skipwhite(ml_get_curline());
            if (cin_is_cpp_namespace(l))
+           {
+           lookfor_cpp_namespace = TRUE;
            amount += ind_cpp_namespace;
+           }
        }
        else
        {
@@ -7097,7 +7100,9 @@
            lookfor_break = TRUE;

        lookfor = LOOKFOR_INITIAL;
-       amount += ind_level;    /* ind_level from start of block */
+       /* ind_level from start of block */
+       if (!lookfor_cpp_namespace)
+           amount += ind_level;
        }
        scope_amount = amount;
        whilelevel = 0;
@@ -7238,7 +7243,7 @@
            {
            if (cont_amount > 0)
                amount = cont_amount;
-           else
+           else if (!lookfor_cpp_namespace)
                amount += ind_continuation;
            }
            else
h-east commented 12 years ago

@mattn nice! たぶん、lookfor_cpp_namespaceの値でind_levelの加算を制御するのは間違いだと思います。

ちゃんと説明してなかったのでここでします。

set cino= (インデントデフォルト >s,N0,+s)で以下のコードを整形すると以下のようなインデントになります。

namespace {
>---template <class T>
>--->---void func() {}
//  ┗→ +s による(継続行)インデント (変数名 ind_continuation) ※
//┗───→ >s による(通常ブロック)インデント (変数名 ind_level)
}

※+sが関数の内側ではないのに設定値分インデントされているのがNG!!


set cino=>s,Ns,+s で以下のコードを整形すると以下のようなインデントになります。

namespace {
>--->---template <class T>
>--->--->---void func() {}
//    ┗→ +s による(継続行)インデント (変数名 ind_continuation) ※
//  ┗───→ Ns による(namespace)インデント (変数名 ind_cpp_namespace)
//┗──────→ >s による(通常ブロック)インデント (変数名 ind_level)
}

※+sが関数の内側ではないのに設定値分インデントされているのがNG!!


set cino=>s,N1,+2 で整形すると分り易いと思います。

つまり、ind_levelの反映可否をnamespace中かどうかで決めてはいけないと思うのです。

mattn commented 12 years ago

僕はこのままソース読むとゲロが出るので兄さんに譲ります。

h-east commented 12 years ago

兄さん( @thinca )、譲られました。

thinca commented 12 years ago

トスしたい。

mattn commented 12 years ago

t0が効かないってのがバグ?

h-east commented 12 years ago

関数の中じゃないのに +s が効いちゃうのがバグです。 :help cino-+

mattn commented 12 years ago

じゃぁこう?

diff -r 484291e1a8da src/misc1.c
--- a/src/misc1.c   Mon Jan 23 20:48:40 2012 +0100
+++ b/src/misc1.c   Wed Jan 25 14:18:04 2012 +0900
@@ -7068,7 +7068,10 @@

            l = skipwhite(ml_get_curline());
            if (cin_is_cpp_namespace(l))
+           {
+           lookfor_cpp_namespace = TRUE;
            amount += ind_cpp_namespace;
+           }
        }
        else
        {
@@ -7238,7 +7243,7 @@
            {
            if (cont_amount > 0)
                amount = cont_amount;
-           else
+           else if (!lookfor_cpp_namespace)
                amount += ind_continuation;
            }
            else
h-east commented 12 years ago

@mattn ++ cool! 報告の件直りました。


※ヘルプの文言に対する個人的な感想 :h cino-+の文言「関数の内」「関数の外」(英語だとinside a function/outside a function)は表現を変えたほうが良いんじゃないかなぁ。 だって人間が判定する「関数の内/外」を実装しようとしたら結構大変なのでキリがないんじゃないかと。 例↓

extern "c" {
    // 関数内じゃないけど cino-+ が効いちゃうよ!
}

必要なら別Issue立てて議論しましょう。

h-east commented 12 years ago

あれ?、ちょっとへんです。

set cino=>s,N1,+2 で整形すると { の位置でインデントが変わります。(この場合は上が正解)

int a(void) {
    int
      a;
}
int a(void)
{
    int
    a;
}
mattn commented 12 years ago

ふむ...

そろそろヒーローが登場↓

mattn commented 12 years ago

2個目のifに何かしら条件が必要...

mattn commented 12 years ago

ちゅうかあれだな。1個目で付けるフラグをlookfor_cpp_namespaceとは別のフラグにすべきだな。

mattn commented 12 years ago

むずい。丸投げ。

h-east commented 12 years ago

@thinca @mattn , C++er&&Vimmer 下のコードを(set cino=設定で) gg=G したときの期待インデントはこれで合ってます? (「関数」の中でのみ継続行インデント(:help cino-+ 参照)が効く)


namespace ns_a {
    int
    val_a;
    void
    func_a()
    {
        int
            val_fa;
    }
    namespace ns_b {
        int
        val_b;
        void
        func_b()
        {
            int
                val_fb;
        }
    }
}

int
val_g;

    void
func_c()
{
    int
        fc_val;
}

    int
main()
{
    ns_a::func_a();
    ns_a::ns_b::func_b();
    func_c();
    return 0;
}
mattn commented 12 years ago

すんません。どういう意味か良く分かりませんが。とりあえずcppのftに↑を貼り付けてset cino=してgg=Gしたらこうなりました。

namespace ns_a {
  int
    val_a;
  void
    func_a()
    {
      int
        val_fa;
    }
  namespace ns_b {
    int
      val_b;
    void
      func_b()
      {
        int
          val_fb;
      }
  }
}

int
val_g;

  void
func_c()
{
  int
    fc_val;
}

  int
main()
{
  ns_a::func_a();
  ns_a::ns_b::func_b();
  func_c();
  return 0;
}
h-east commented 12 years ago

インデント決定ロジックに「後方↑最寄に { があれば関数の中と判断する」というのがあります。(:help cino-+) これを「後方↑最寄に { があれば関数の中(※但し、namespaceの{は除く)」というロジックに変更すればこのissueは解決しますよね? という確認で、ロジック変更した場合の想定インデントとして https://github.com/vim-jp/issues/issues/140#issuecomment-5675348 を提示しました。

「No!」の場合は仕様を提示して頂戴( @thinca さん、C++er&&Vimmer) 仮に、「Yes!」と言われてもムズいので年金生活に入るまで対応できないすけどねw

thinca commented 12 years ago

これは、「期待値はこれでいいのか」って確認ですよね。 改めて動作を見てみたんですが、

int
val_a;

これに関しては現在トップレベルでもこのようにインデントされますね。ただ、

double val_a,
       val_b;

とすると前の行に合わせてインデントされたりして、この仕様がどこから来てるかまだちょっと把握しきれてません…(設定多い!)。 ちゃんと把握してない段階でこんなこと言うのもアレですが、もしかしたら前者の挙動自体がバグかもしれませんね。:hlep cino-+ でも、関数の外側ではインデントされない、とは書いてませんし。 で、肝心の期待値はどうなのかって話ですが、namespace 内はトップレベルと同じ扱いって意味ではそれでいいと思います。 ところで現状の動作私も試しめしたが @mattn さんと結果が違う件。

namespace ns_a {
  int
    val_a;
  void
    func_a()
    {
      int
        val_fa;
    }
  namespace ns_b {
    int
      val_b;
    void
      func_b()
      {
        int
          val_fb;
      }
  }
}

int
val_g;

  void
func_c()
{
  int
    fc_val;
}

  int
main()
{
  ns_a::func_a();
  ns_a::ns_b::func_b();
  func_c();
  return 0;
}
h-east commented 12 years ago

これは、「期待値はこれでいいのか」って確認ですよね。

はい。

double val_a,
       val_b;

とすると前の行に合わせてインデントされたりして、この仕様がどこから来てるかまだちょっと把握しきれてません…(設定多い!)。

前行が , で終わっている場合は cino-+ とは別の(cinoで制御できない)処理がなされます。 src/misc1.c : 8212 ~ 8256 (8252行のcin_first_id_amount()) ですから今回のissueとは無関係です。

:hlep cino-+ でも、関数の外側ではインデントされない、とは書いてませんし。

「関数の内側では~インデントする。」と書いてあるのにそんなこと言ったら「秩序とは何か」って話になって通常の生活出来なくなりますよ。

ところで現状の動作私も試しめしたが @mattn さんと結果が違う件。

どこが違うかワカラナイ(`ェ´)ピャー

thinca commented 12 years ago

どこが違うかワカラナイ(`ェ´)ピャー

あれ。ああ、本当だ。github.vim で見たせいで Tab のインデントが狂ってたんですね…さーせん。

mattn commented 12 years ago

むー...

thinca commented 12 years ago

「関数の内側では~インデントする。」と書いてあるのにそんなこと言ったら「秩序とは何か」って話になって通常の生活出来なくなりますよ。

英語版見て把握。

:help cino-+@en

Indent a continuation line (a line that spills onto the next)
inside a function N additional characters.  (default
'shiftwidth').

:help cino-+@ja

継続行 (次の行へと分割された行) をインデントする。
関数の内側では N 文字分インデントする。 (省略値 'shiftwidth')

日本語の意味変わっちゃってますね…。これだと、まず無条件でインデントは行って、さらに関数の内側では…と言う意味に取れる。

thinca commented 12 years ago

訳修正しました。 https://github.com/vim-jp/vimdoc-ja/commit/ac94994dcc148ff06bde57f171a61715d0690222

h-east commented 12 years ago

@thinca speedy wonder!

mattn commented 12 years ago

母:「学校の宿題もこれくらい頑張ってくれたらいいんだけどね」

thinca commented 12 years ago

宿題やってなくてごめんなさい!(なんとなく謝っておく)

h-east commented 12 years ago

母:「期待値はこれでいいのかい?3文字以内で答えるんだよ」 https://github.com/vim-jp/issues/issues/140#issuecomment-5675348

mattn commented 12 years ago

期待値を知るにはオプションのデフォルト値がどう動作すべきか知る必要があるので、即答出来ないかと思います。 どっちかっていうと、現状namespace下のインデントをnamespace外のインデントと同じにする術がないので欲しいってところです。(僕は)

h-east commented 12 years ago

@mattn

期待値を知るにはオプションのデフォルト値がどう動作すべきか知る必要があるので、即答出来ないかと思います。

うーん、別に意地悪な問題出して「間違ってやんのプププ」とかするつもりはないんですよ。

(私がコメントの順番をミスってますけど、言いたかったのはこれ↓) https://github.com/vim-jp/issues/issues/140#issuecomment-5684391

インデント決定ロジックに「後方↑最寄に { があれば関数の中と判断する」というのがあります。(:help cino-+) これを「後方↑最寄に { があれば関数の中(※但し、namespaceの{は除く)」というロジックに変更すればこのissueは解決しますよね?

cino-+をロジックをこう変えたら良いですよね?んで、変えた場合のインデントはこれであってますよね? ということなんですけど伝わりません? 例が悪かったかなぁ。

どっちかっていうと、現状namespace下のインデントをnamespace外のインデントと同じにする術がないので欲しいってところです。(僕は)

これはcino-+ のロジックを変えずに別にオプションを新設した方が良いということですか?

mattn commented 12 years ago

これはcino-+ のロジックを変えずに別にオプションを新設した方が良いということですか?

いえ、fixでもなんでも良いです。namespaceと関数との区別に必要なら新設もありだと思いますが。

thinca commented 12 years ago

私も最終的に欲しい挙動は @mattn さんと同じで「namespace下のインデントをnamespace外のインデントと同じにする」なんですが、 どちらにしてもこの cino-+ が namespace を関数内と認識してしまう問題はバグで直さないとどうしようもないと思います。 これを直せば最終的に欲しい挙動が手に入るかはわからないですが、それはやるならまた別 issue かな、と。

h-east commented 12 years ago

了解です。 この件って明確にbugと呼ぶべきではない気がしてきたのでラベルをdiscusに変更しました。 あとちょっと疲れたのでassignを外しました。 真夏に逢いましょう。

mattn commented 12 years ago

書きかけのパッチとかあります?

h-east commented 12 years ago

@mattn すんません返事遅くなりました。パッチないです。

mattn commented 12 years ago

了解す。 時間あったら僕もみます。