Re: [ELPA] New Package: p4_16-mode

[Philip Kaludercic]

Soham Gumaste <sohamg2@gmail.com> writes:

> Hello Emacs Devs,
>
> I am writing to ask that my package "p4_16-mode.el" be added to ELPA.
> The repo is at the following link:
>
> https://github.com/SohamG/p4_16-mode.el

Here are a few quick changes I'd propose:

diff --git a/p4_16-mode.el b/p4_16-mode.el
index 23c2825..f9d146b 100644
--- a/p4_16-mode.el
+++ b/p4_16-mode.el
@@ -3,26 +3,42 @@
 ;; Author: Soham S Gumaste <sohamg2@gmail.com>
 ;; Maintainer: Soham S Gumaste <sohamg2@gmail.com>
 ;; Created: 11 Nov 2023
-;; Original Source: 
https://github.com/p4lang/tutorials/blob/master/vm/p4_16-mode.el
-;; Original License: Apache 2.0
-;; Original Author: Vladimir Gurevich <vladimir.gurevich@barefootnetworks.com>
-;; Modifications bylicensed under the same license OR the MIT License.
+;; Modifications bilicensed under the same license OR the MIT License.
 
 ;; Keywords: languages p4_16
 
 ;; This file is not part of GNU Emacs.
 
-;; This file is free software…
-;; …
-;; along with this file.  If not, see <https://www.gnu.org/licenses/>.
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 ;;; Commentary:
+
 ;; P4 (Programming Protocol Independent Packet Processors) is a domain
 ;; specific language designed to program network fabric devices.
 ;; This mode has preliminary support for P4_16. It covers the core language,
 ;; but it is not clear yet, how we can highlight the indentifiers, defined
-;; for a particular architecture. Core library definitions are included
+;; for a particular architecture.  Core library definitions are included
 
+;;; History:
+
+;; The headers above have no special meaning, and could also be
+;; re-written in prose under a special section.  Also, was the package
+;; really created just a few days ago?
+
+;; Original Source: 
https://github.com/p4lang/tutorials/blob/master/vm/p4_16-mode.el
+;; Original License: Apache 2.0
+;; Original Author: Vladimir Gurevich <vladimir.gurevich@barefootnetworks.com>
 
 ;;; Code:
 (defvar p4_16-mode-hook nil)
@@ -30,46 +46,46 @@
 ;; Define the keymap (for now it is pretty much default)
 (defvar p4_16-mode-map
   (let ((map (make-keymap)))
-    (define-key map "\C-j" 'newline-and-indent)
+    (define-key map (kbd "C-j") 'newline-and-indent)
     map)
-  "Keymap for P4_16 major mode")
+  "Keymap for P4_16 major mode.")
 
-;; Syntactic HighLighting
+;;; Syntactic Highlighting
 
-;; Main keywords (declarations and operators)
+;;;; Main keywords (declarations and operators)
 (defconst p4_16-keywords
-      '("action" "apply"
-        "control"
-        "default"
-        "else" "enum" "extern" "exit"
-        "header" "header_union"
-        "if"
-        "match_kind"
-        "package" "parser"
-        "return"
-        "select" "state" "struct" "switch"
-        "table"  "transition" "tuple" "typedef"
-        "verify")
-      "P4_16 Standard Keywords")
+  '("action" "apply"
+    "control"
+    "default"
+    "else" "enum" "extern" "exit"
+    "header" "header_union"
+    "if"
+    "match_kind"
+    "package" "parser"
+    "return"
+    "select" "state" "struct" "switch"
+    "table"  "transition" "tuple" "typedef"
+    "verify")
+  "P4_16 Standard Keywords.")
 
 (defconst p4_16-annotations
   '("@name" "@metadata" "@alias")
-  "P4_16 Standard Annotations")
+  "P4_16 Standard Annotations.")
 
 (defconst p4_16-attributes
-      '("const" "in" "inout" "out"
-        ;; Tables
-        "key" "actions" "default_action" "entries" "implementation"
-        "counters" "meters")
-      "P4_16 Object Attributes and Access Specifiers")
+  '("const" "in" "inout" "out"
+    ;; Tables
+    "key" "actions" "default_action" "entries" "implementation"
+    "counters" "meters")
+  "P4_16 Object Attributes and Access Specifiers.")
 
 (defconst p4_16-variables
   '("packet_in" "packet_out")
-  "P4_16 Packet Types")
+  "P4_16 Packet Types.")
 
 (defconst p4_16-operations
   '("&&&" ".." "++" "?" ":")
-  "P4_16 Operators")
+  "P4_16 Operators.")
 
 (defconst p4_16-constants
   '(;; Don't care
@@ -83,11 +99,11 @@
     "exact" "ternary" "lpm" "range"
     ;; We can add constants for supported architectures here
     )
-  "P4_16 Standard Constants")
+  "P4_16 Standard Constants.")
 
 (defconst p4_16-types
   '("bit" "bool" "int" "varbit" "void" "error")
-  "P4_16 Standard Datatypes")
+  "P4_16 Standard Datatypes.")
 
 (defconst p4_16-primitives
   '(;; Header methods
@@ -102,37 +118,36 @@
     "accept" "reject"
     ;; misc
     "NoAction")
-  "P4_16 Standard Primitives")
+  "P4_16 Standard Primitives.")
 
 (defconst p4_16-cpp
-      '("#include"
-        "#define" "#undef"
-        "#if" "#ifdef" "#ifndef"
-        "#elif" "#else"
-        "#endif"
-        "defined"
-        "#line" "#file"))
+  '("#include"
+    "#define" "#undef"
+    "#if" "#ifdef" "#ifndef"
+    "#elif" "#else"
+    "#endif"
+    "defined"
+    "#line" "#file"))
 
 (defconst p4_16-cppwarn
-      '("#error" "#warning"))
+  '("#error" "#warning"))
 
 ;; Optimize the strings
-(setq p4_16-keywords-regexp    (regexp-opt p4_16-keywords   'words))
-(setq p4_16-annotations-regexp (regexp-opt p4_16-annotations     1))
-(setq p4_16-attributes-regexp  (regexp-opt p4_16-attributes 'words))
-(setq p4_16-variables-regexp   (regexp-opt p4_16-variables  'words))
-(setq p4_16-operations-regexp  (regexp-opt p4_16-operations 'words))
-(setq p4_16-constants-regexp   (regexp-opt p4_16-constants  'words))
-(setq p4_16-types-regexp       (regexp-opt p4_16-types      'words))
-(setq p4_16-primitives-regexp  (regexp-opt p4_16-primitives 'words))
-(setq p4_16-cpp-regexp         (regexp-opt p4_16-cpp        1))
-(setq p4_16-cppwarn-regexp     (regexp-opt p4_16-cppwarn    1))
-
+(defvar p4_16-keywords-regexp    (regexp-opt p4_16-keywords   'words))
+(defvar p4_16-annotations-regexp (regexp-opt p4_16-annotations     1))
+(defvar p4_16-attributes-regexp  (regexp-opt p4_16-attributes 'words))
+(defvar p4_16-variables-regexp   (regexp-opt p4_16-variables  'words))
+(defvar p4_16-operations-regexp  (regexp-opt p4_16-operations 'words))
+(defvar p4_16-constants-regexp   (regexp-opt p4_16-constants  'words))
+(defvar p4_16-types-regexp       (regexp-opt p4_16-types      'words))
+(defvar p4_16-primitives-regexp  (regexp-opt p4_16-primitives 'words))
+(defvar p4_16-cpp-regexp         (regexp-opt p4_16-cpp        1))
+(defvar p4_16-cppwarn-regexp     (regexp-opt p4_16-cppwarn    1))
 
 ;; create the list for font-lock.
 ;; each category of keyword is given a particular face
 (defconst p4_16-font-lock-keywords
-  (list
+  (list                                        ;Perhaps use backquoting?
    (cons p4_16-cpp-regexp         font-lock-preprocessor-face)
    (cons p4_16-cppwarn-regexp     font-lock-warning-face)
    (cons p4_16-types-regexp       font-lock-type-face)
@@ -154,16 +169,16 @@
    (cons "\\([^_A-Za-z][+-]?\\([0-9]+w\\)?[0-9]+\\)"    
font-lock-constant-face)
    ;;(cons "\\(\\w*\\)"        font-lock-variable-name-face)
    )
-  "Default Highlighting Expressions for P4_16")
+  "Default Highlighting Expressions for P4_16.")
 
 (defvar p4_16-mode-syntax-table
   (let ((st (make-syntax-table)))
     (modify-syntax-entry  ?_  "w"      st)
     (modify-syntax-entry  ?/  ". 124b" st)
     (modify-syntax-entry  ?*  ". 23"   st)
-    (modify-syntax-entry  ?\n  "> b"   st)
+    (modify-syntax-entry  ?\n "> b"    st)
     st)
-  "Syntax table for p4_16-mode")
+  "Syntax table for p4_16-mode.")
 
 ;;; Indentation
 (defvar p4_16-indent-offset 4
@@ -192,7 +207,7 @@
 
 ;; Put everything together
 (defun p4_16-mode ()
-  "Major mode for editing P4_16 programs"
+  "Major mode for editing P4_16 programs."
   (interactive)
   (kill-all-local-variables)
   (set-syntax-table p4_16-mode-syntax-table)
@@ -201,8 +216,8 @@
   (set (make-local-variable 'indent-line-function) 'p4_16-indent-line)
   (setq major-mode 'p4_16-mode)
   (setq mode-name "P4_16")
-  (with-eval-after-load "xcscope" (cscope-minor-mode))
+  (when (fboundp 'cscope-minor-mode) (cscope-minor-mode))
   (run-hooks 'p4_16-mode-hook))
 
-;; The most important line
 (provide 'p4_16-mode)
+;;; p4_16-mode.el ends here

Also, would it be imaginable to rename it to p4-16-mode?  The mid-symbol
underscore looks unusual in a Lisp context

> Summary:
> P4 (Programming Protocol Independent Packet Processors) is a domain
> specific language used to program network fabric devices like NICs or
> switches. The project can be found at https://p4.org . This package
> adds a major mode for this language. This mode was initially a part of
> the github repo "p4lang/tutorials" (Apache 2.0). I have added some
> polish, and obtained permission from the repo maintainers to submit
> here.

I suppose you want to add the package to NonGNU ELPA then?

> I do not have write access to the ELPA git repo. Please let me know if
> I need to make any modifications.
>
> Thanks