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

We're using lots of "internal" cc-mode functions #128

Closed
josteink opened this issue Mar 13, 2018 · 7 comments
Closed

We're using lots of "internal" cc-mode functions #128

josteink opened this issue Mar 13, 2018 · 7 comments

Comments

@josteink
Copy link
Collaborator

According to Alan Mackenzie, we're using functions considered internal "by convention":

There's a convention in CC Mode that functions, macros, and variables with a doc string are regarded as part of an "API" to derived modes, but objects with merely a "doc comment" are regarded as internal to CC Mode, and much less secure against random changes.

Doing a quick skim of the csharp-mode codebase, that includes at least the following:

  • c-safe
  • c-put-character-property
  • c-put-font-lock-face
  • c-fontify-types-and-refs
  • c-forward-keyword-clause
  • c-font-lock-declarators

Not to mention we monkey-patch/advice c-inside-bracelist-p and c-forward-objc-directive.

Some of these are probably safer to use than others (pun intended), but it's still quite a list.

What should we do about it? Should we do something about it? Or just hang around until i breaks again?

Opinions? Initiatives?

@mookid
Copy link
Contributor

mookid commented Oct 10, 2018

I have written the following script, that classifies the usage of cc-mode api:

(defun suspicious-forms (form)
  (cond ((consp form)
         (append (suspicious-forms (car form))
                 (suspicious-forms (cdr form))))
        ((and (symbolp form) (string-prefix-p "c-" (symbol-name form)))
         (list form))))

(defun skip-comments ()
  (let ((previous-point (point)))
    (while
        (progn
          (forward-comment 1)
          (< (point) previous-point))
      (setq previous-point (point)))))

(defun all-buffer-forms (buffer)
  (with-current-buffer buffer
    (let ((acc nil)
          (form nil))
      (goto-char (point-min))
      (while
          (and
           (progn
             (skip-comments)
             (< (point) (point-max)))
           (setq form (read (current-buffer))))
        (push form acc))
      acc)))

(defun classify-c-mode-symbols (symbols)
  (let ((official-api nil)              ; have a docstring
        (non-official-api nil))
    (dolist (symbol symbols)
      (push symbol
            (if (documentation-property symbol 'variable-documentation)
                official-api
              non-official-api)))
    (list :official-api (sort official-api 'string<)
          :non-official-api (sort non-official-api 'string<))))



The results of (classify-c-mode-symbols (delete-dups (suspicious-forms (all-buffer-forms "csharp-mode.el")))) are the following:


(:official-api
  (c-at-vsemi-p-fn c-basic-offset c-comment-only-line-offset
  c-default-style c-file-style c-identifier-key
  c-identifier-syntax-modifications c-multiline-string-start-char
  c-nonlabel-token-key c-offsets-alist c-opt-cpp-macro-define
  c-opt-cpp-prefix c-opt-op-identifier-prefix
  c-recognize-<>-arglists c-recognize-paren-inexpr-blocks
  c-special-brace-lists c-string-escaped-newlines c-symbol-chars
  c-symbol-key)
  :non-official-api
  (c-add-language c-add-style c-alnum c-assignment-operators
  c-at-statement-start-p c-backward-syntactic-ws
  c-backward-token-2 c-basic-matchers-after
  c-basic-matchers-before c-before-label-kwds c-before-label-re
  c-beginning-of-statement-1 c-block-prefix-disallowed-chars
  c-block-stmt-1-kwds c-block-stmt-2-kwds c-block-stmt-kwds
  c-brace-list-decl-kwds c-brace-list-key c-class-decl-kwds
  c-class-key c-colon-type-list-kwds c-common-init
  c-constant-face-name c-constant-kwds c-cpp-expr-directives
  c-cpp-matchers c-cpp-message-directives
  c-crosses-statement-barrier-p c-decl-block-key c-decl-id-start
  c-define-abbrev-table c-down-list-forward
  c-electric-continued-statement c-font-lock-declarators
  c-font-lock-invalid-string c-fontify-types-and-refs
  c-forward-keyword-clause c-forward-objc-directive
  c-forward-sexp c-forward-syntactic-ws c-forward-token-2
  c-in-literal c-indent-line-or-region c-indent-region
  c-inexpr-block-kwds c-inexpr-brace-list-kwds
  c-inexpr-class-kwds c-init-language-vars c-initialize-cc-mode
  c-inside-bracelist-p c-keyword-member c-keyword-sym c-keywords
  c-keywords-regexp c-label-face-name c-label-kwds c-lambda-kwds
  c-lang-const c-lang-defconst c-lineup-C-comments
  c-lineup-arglist c-lineup-arglist-intro-after-paren
  c-lineup-comment c-lineup-dont-change c-lineup-inexpr-block
  c-lineup-multi-inher c-lineup-streamop c-lineup-template-args
  c-looking-at-inexpr-block c-looking-at-special-brace-list
  c-major-mode-is c-make-font-lock-search-function
  c-make-inherited-keymap c-make-keywords-re
  c-make-mode-syntax-table c-matchers-1 c-matchers-2 c-matchers-3
  c-modifier-kwds c-most-enclosing-brace c-on-identifier
  c-operators c-opt-after-id-concat-key
  c-opt-inexpr-brace-list-key c-other-block-decl-kwds
  c-other-kwds c-overloadable-operators c-paren-type-kwds c-point
  c-preprocessor-face-name c-primary-expr-kwds
  c-primitive-type-kwds c-promote-possible-types
  c-protection-kwds c-put-char-property c-put-font-lock-face
  c-ref-list-kwds c-regular-keywords-regexp c-safe
  c-simple-stmt-kwds c-skip-comments-and-strings
  c-string-limit-regexp c-type c-type-list-kwds
  c-type-modifier-kwds c-type-prefix-kwds c-typeless-decl-kwds
  c-update-modeline))


@josteink
Copy link
Collaborator Author

Thanks for the input! That's definitely a more comprehensive effort than I've put in previously.

My initial reaction to your post was "Oh my! That's terrible!", but upon closer inspection I realize I may need some help interpreting that data.

What does "suspicious form" technically entail, and does usage of "non-official-api" mean that these methods are risky to use w.r.t. compatibility?

What approach and remedy would you suggest based on this analysis?

@mookid
Copy link
Contributor

mookid commented Oct 11, 2018

What does "suspicious form" technically entail, and does usage of "non-official-api" mean that these methods are risky to use w.r.t. compatibility?

I just assume that cc-mode symbols are exactly those prefixed by "c-" (probably not a too bad approximation). Concerning "non-official-api", I just literally interpreted the quote of the first message. Being unfamiliar with cc-mode, I don't really know where to go from here.

The most robust approach would probably be to remove completely dependency on cc-mode, and do what most emacs modes do concerning syntax highlighting, indentation etc.

@josteink
Copy link
Collaborator Author

The most robust approach would probably be to remove completely dependency on cc-mode, and do what most Emacs modes do concerning syntax highlighting, indentation etc.

In theory I agree, but that would effectively be a full rewrite, and unfortunately, I simply do not have the time to even estimate the scope of such an effort.

I've previously stated that csharp-mode is currently in maintenance mode, where most effort is directed at fixing bugs and not making major changes.

Unless anyone can summon a much more active co-maintainer, I'm afraid that's how things will remain for the foreseeable future.

@mookid
Copy link
Contributor

mookid commented Oct 26, 2018

edit: the script actually discards functions. here is the fix and fixed diagnostic:

(defun classify-c-mode-symbols (symbols)
  (let ((official-api nil)              ; have a docstring
        (non-official-api nil))
    (dolist (symbol symbols)
      (push symbol
            (if (or
		 (documentation-property symbol 'variable-documentation)
		 (and
		  (fboundp symbol)
		  (documentation symbol)))
                official-api
              non-official-api)))
    (list :official-api (sort official-api 'string<)
          :non-official-api (sort non-official-api 'string<))))








(:official-api
  (c-add-language c-add-style c-at-statement-start-p
  c-at-vsemi-p-fn c-backward-syntactic-ws c-backward-token-2
  c-basic-offset c-beginning-of-statement-1
  c-comment-only-line-offset c-common-init
  c-crosses-statement-barrier-p c-default-style
  c-down-list-forward c-electric-continued-statement c-file-style
  c-forward-sexp c-forward-syntactic-ws c-forward-token-2
  c-identifier-key c-identifier-syntax-modifications c-in-literal
  c-indent-line-or-region c-indent-region c-initialize-cc-mode
  c-lang-const c-lang-defconst c-lineup-C-comments
  c-lineup-arglist c-lineup-arglist-intro-after-paren
  c-lineup-comment c-lineup-dont-change c-lineup-inexpr-block
  c-lineup-multi-inher c-lineup-streamop c-lineup-template-args
  c-major-mode-is c-make-keywords-re
  c-multiline-string-start-char c-nonlabel-token-key
  c-offsets-alist c-on-identifier c-opt-cpp-macro-define
  c-opt-cpp-prefix c-opt-op-identifier-prefix c-point
  c-recognize-<>-arglists c-recognize-paren-inexpr-blocks
  c-special-brace-lists c-string-escaped-newlines c-symbol-chars
  c-symbol-key)
  :non-official-api
  (c-alnum c-assignment-operators c-basic-matchers-after
  c-basic-matchers-before c-before-label-kwds c-before-label-re
  c-block-prefix-disallowed-chars c-block-stmt-1-kwds
  c-block-stmt-2-kwds c-block-stmt-kwds c-brace-list-decl-kwds
  c-brace-list-key c-class-decl-kwds c-class-key
  c-colon-type-list-kwds c-constant-face-name c-constant-kwds
  c-cpp-expr-directives c-cpp-matchers c-cpp-message-directives
  c-decl-block-key c-decl-id-start c-define-abbrev-table
  c-font-lock-declarators c-font-lock-invalid-string
  c-fontify-types-and-refs c-forward-keyword-clause
  c-forward-objc-directive c-inexpr-block-kwds
  c-inexpr-brace-list-kwds c-inexpr-class-kwds
  c-init-language-vars c-inside-bracelist-p c-keyword-member
  c-keyword-sym c-keywords c-keywords-regexp c-label-face-name
  c-label-kwds c-lambda-kwds c-looking-at-inexpr-block
  c-looking-at-special-brace-list
  c-make-font-lock-search-function c-make-inherited-keymap
  c-make-mode-syntax-table c-matchers-1 c-matchers-2 c-matchers-3
  c-modifier-kwds c-most-enclosing-brace c-operators
  c-opt-after-id-concat-key c-opt-inexpr-brace-list-key
  c-other-block-decl-kwds c-other-kwds c-overloadable-operators
  c-paren-type-kwds c-preprocessor-face-name c-primary-expr-kwds
  c-primitive-type-kwds c-promote-possible-types
  c-protection-kwds c-put-char-property c-put-font-lock-face
  c-ref-list-kwds c-regular-keywords-regexp c-safe
  c-simple-stmt-kwds c-skip-comments-and-strings
  c-string-limit-regexp c-type c-type-list-kwds
  c-type-modifier-kwds c-type-prefix-kwds c-typeless-decl-kwds
  c-update-modeline))

@josteink
Copy link
Collaborator Author

This is pretty much going to be solved by the rework-branch and I just can't tell you guys how happy I am about that! 🍾 🎆

@josteink
Copy link
Collaborator Author

This alone is worth a celebration! 😄

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

No branches or pull requests

3 participants