Add rust `edition` feature #12

MahadMuhammad · 2024-08-17T10:16:17Z

Fixes: Add parsing additional options #8

Depends on: Use global static Regex variable for pattern matching #10

src/known-directives.rs

Signed-off-by: Muhammad Mahad <[email protected]>

Add `regex` and exported it for other files Signed-off-by: Muhammad Mahad <[email protected]>

Signed-off-by: Muhammad Mahad <[email protected]>

Fixes: Rust-GCC#9 Signed-off-by: Muhammad Mahad <[email protected]>

Signed-off-by: Muhammad Mahad <[email protected]>

If the test source file doesn't contain any headers, or that are not added till now then the header value will be None and becomes panic Signed-off-by: Muhammad Mahad <[email protected]>

P-E-P

A lot of great things in there, most of my comments are nitpicks because you did really great 😆

P-E-P · 2024-10-15T08:27:12Z

src/cli.rs

+    /// clap reports most development errors as `debug_assert!`s
+    /// See this for more details, [here](https://docs.rs/clap/4.5.15/clap/_derive/_tutorial/chapter_4/index.html)


I'm not sure a doc comment is required ? A regular comment might be enough

P-E-P · 2024-10-15T08:31:14Z

src/errors.rs

+    std::{fmt, str::FromStr},
+};
+
+// https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex


Suggested change

// https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex

/// Macro used to lazily create a new regex the first time it is invoked.

///

/// # Arguments

///

/// * `re` - The regex literal string used to build the automaton

///

/// # Example

///

/// ```rust

/// assert!(regex!(r"\w").is_match(" "));

/// ```

///

/// Taken from here https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex

Macros can be tricky to understand, I'd like this to be more thoroughly documented. A doctest example even provides a great insight (the example should compile & run when you launch cargo test).

P-E-P · 2024-10-15T08:32:23Z

src/errors.rs

@@ -60,11 +72,14 @@ pub struct Error {
    /// What kind of message we expect (e.g., warning, error, suggestion).
    /// `None` if not specified or unknown message kind.
    pub kind: Option<RustcErrorKind>,
+    ///Note: if we are loading this from rustc source file, this might be incomplete


Suggested change

///Note: if we are loading this from rustc source file, this might be incomplete

/// Note: if we are loading this from rustc source file, this might be incomplete

P-E-P · 2024-10-15T08:33:04Z

src/errors.rs

+    /// Formats the `Error` for display according to `DejaGnu` format
+    /// See `DejaGnu` documentation [here](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)


Suggested change

/// Formats the `Error` for display according to `DejaGnu` format

/// See `DejaGnu` documentation [here](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)

/// Formats the `Error` for display according to `DejaGnu` format

/// See [`DejaGnu` documentation](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)

P-E-P · 2024-10-15T08:35:50Z

src/errors.rs

    pub msg: String,
    pub error_code: Option<String>,
 }

 impl fmt::Display for Error {
+    /// Formats the `Error` for display according to `DejaGnu` format
+    /// See `DejaGnu` documentation [here](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        use RustcErrorKind::*;



let error_type = match &self.kind { Some(Help) => "help", Some(Note) => "dg-note", Some(Suggestion) => "suggestion", Some(Warning) => "dg-warning", Some(Error) | None => "dg-error", };

P-E-P · 2024-10-15T08:52:09Z

src/header.rs

+        Some(HeaderLine {
+            line_number: line_number + 1, // 1 based-indexed instead of zero based
+            _directive: "edition",
+            dejagnu_header: to_dejagnu_edition(edition.unwrap().as_str()),


Why can we unwrap edition safely without panicking here ?

P-E-P · 2024-10-15T08:53:54Z

src/header.rs

+            panic!(
+                "malformed condition directive: expected `{comment}[foo]`, found `{original_line}`"
+            )


I'm not thrilled about this panic here

P-E-P · 2024-10-15T08:55:32Z

src/known-directives.rs

@@ -0,0 +1,233 @@
+// Copied from https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/command-list.rs


We want this source to appear in the documentation

P-E-P · 2024-10-15T08:56:50Z

src/main.rs

@@ -1,8 +1,13 @@
-use anyhow::{Context, Result};
-use clap::Parser;
+//! The main entry point of the program.


No need to comment main.rs like this :) File is called main.rs and has a main function it is fairly obvious is the main entry point.

P-E-P · 2024-10-15T09:01:18Z

src/transform.rs

+            // TODO: This is not the efficient way to find respective line number
+            for error in errors.iter() {
+                // Checking the original line number
+                if (error.line_num as i32 - error.relative_line_num) != line_num as i32 {


I'd prefer if you could use either the From trait or the TryFrom trait (or their Into counterpart) instead of a cast because I don't like silent errors that morph into bugs.

CohenArthur reviewed Aug 19, 2024

View reviewed changes

src/known-directives.rs Outdated Show resolved Hide resolved

src/known-directives.rs Outdated Show resolved Hide resolved

MahadMuhammad added 15 commits August 21, 2024 23:17

chore: Add once_cell as dependency

06cbf37

Signed-off-by: Muhammad Mahad <[email protected]>

Add regex! Macro

54a42df

Add `regex` and exported it for other files Signed-off-by: Muhammad Mahad <[email protected]>

Refactor: Use regex! for Pattern Matching

a645a19

Signed-off-by: Muhammad Mahad <[email protected]>

Fix ErrCode parsing on incomplete source file msg

f1b8490

Fixes: Rust-GCC#9 Signed-off-by: Muhammad Mahad <[email protected]>

Refactor imports in cli.rs and main.rs

86cf8ae

Signed-off-by: Muhammad Mahad <[email protected]>

doc: Add doc comments

8bd6afe

Signed-off-by: Muhammad Mahad <[email protected]>

Add header module

fb038fc

Signed-off-by: Muhammad Mahad <[email protected]>

Add HeaderLine struct to store header info

0a53af7

Signed-off-by: Muhammad Mahad <[email protected]>

Add parse_edition functions

2a84c71

Signed-off-by: Muhammad Mahad <[email protected]>

Add function for dejagnu edition format

81c4991

Signed-off-by: Muhammad Mahad <[email protected]>

Add logic for tokenizing header line

70bf66c

Add function to store HeaderLine info

e180b3b

Signed-off-by: Muhammad Mahad <[email protected]>

Add exhaustive names of all known directives

dec1beb

Signed-off-by: Muhammad Mahad <[email protected]>

feat/Add rust edition functionality

d71ddfe

Signed-off-by: Muhammad Mahad <[email protected]>

remove rustc internal comments

15d5640

Signed-off-by: Muhammad Mahad <[email protected]>

MahadMuhammad force-pushed the feat/add-additional-options branch 2 times, most recently from 2a82d33 to 15d5640 Compare August 21, 2024 18:34

MahadMuhammad added 2 commits August 21, 2024 23:57

Refactor header line condition

e561a01

Signed-off-by: Muhammad Mahad <[email protected]>

Refactor header line condition to avoid None unwrap

8a33157

If the test source file doesn't contain any headers, or that are not added till now then the header value will be None and becomes panic Signed-off-by: Muhammad Mahad <[email protected]>

MahadMuhammad requested a review from CohenArthur September 9, 2024 14:12

P-E-P approved these changes Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rust `edition` feature #12

Add rust `edition` feature #12

MahadMuhammad commented Aug 17, 2024

P-E-P left a comment

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

P-E-P Oct 15, 2024

		/// clap reports most development errors as `debug_assert!`s
		/// See this for more details, [here](https://docs.rs/clap/4.5.15/clap/_derive/_tutorial/chapter_4/index.html)

-// https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex
+/// Macro used to lazily create a new regex the first time it is invoked.
+///
+/// # Arguments
+///
+/// * `re` - The regex literal string used to build the automaton
+///
+/// # Example
+///
+/// ```rust
+/// assert!(regex!(r"\w").is_match(" "));
+/// ```
+///
+/// Taken from here https://docs.rs/once_cell/1.19.0/once_cell/#lazily-compiled-regex

	///Note: if we are loading this from rustc source file, this might be incomplete
	/// Note: if we are loading this from rustc source file, this might be incomplete

		/// Formats the `Error` for display according to `DejaGnu` format
		/// See `DejaGnu` documentation [here](https://gcc.gnu.org/onlinedocs/gccint/testsuites/directives-used-within-dejagnu-tests/syntax-and-descriptions-of-test-directives.html)

		@@ -0,0 +1,233 @@
		// Copied from https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/command-list.rs

Add rust edition feature #12

Are you sure you want to change the base?

Add rust edition feature #12

Conversation

MahadMuhammad commented Aug 17, 2024

P-E-P left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add rust `edition` feature #12

Add rust `edition` feature #12