Cleanup TODO items (#2285)

Remove TODO comments from code and add them as Postponed items to the
tracking PR.

Author: orpuente-MS

compiler/qsc_qasm3/src/compiler.rs

@@ -243,8 +243,8 @@ impl QasmCompiler {
 
         let ast_ty = map_qsharp_type_to_ast_ty(&output_ty);
         signature.output = format!("{output_ty}");
-        // TODO: This can create a collision on multiple compiles when interactive
-        // We also have issues with the new entry point inference logic
+        // This can create a collision on multiple compiles when interactive
+        // We also have issues with the new entry point inference logic.
         let input_desc = input
             .iter()
             .flat_map(|s| {
@@ -873,11 +873,8 @@ impl QasmCompiler {
         let symbol = &self.symbols[stmt.symbol_id];
 
         // input decls should have been pushed to symbol table,
-        // but should not be the stmts list.
-        // TODO: This may be an issue for tooling as there isn't a way to have a forward
-        // declared varible in Q#.
+        // but should not be in the stmts list.
         if symbol.io_kind != IOKind::Output {
-            //self.push_semantic_error(SemanticErrorKind::InvalidIODeclaration(stmt.span));
             return None;
         }
 

compiler/qsc_qasm3/src/parser/ast.rs

@@ -16,11 +16,6 @@ use std::{
     rc::Rc,
 };
 
-// TODO: Profile this with iai-callgrind in a large OpenQASM3
-//       sample to verify that is actually faster than using Vec<T>.
-//       Even though Box<T> uses less stack space, it reduces cache
-//       locality, because now you need to be jumping around in
-//       memory to read contiguous elements of a list.
 /// An alternative to `Vec<T>` that uses less stack space.
 pub(crate) type List<T> = Box<[Box<T>]>;
 

compiler/qsc_qasm3/src/parser/expr.rs

@@ -51,23 +51,12 @@ enum OpKind {
     Index,
 }
 
-// TODO: This seems to be an unnecessary wrapper.
-//       OpName::Keyword is never used.
-//       Consider removing.
 #[derive(Clone, Copy)]
 enum OpName {
     Token(TokenKind),
     Keyword(Keyword),
 }
 
-// TODO: This seems to be an unnecessary wrapper.
-//       We ended up removing the OpContext::Stmt variant.
-//       Consider removing.
-#[derive(Clone, Copy)]
-enum OpContext {
-    Precedence(u8),
-}
-
 #[derive(Clone, Copy)]
 enum Assoc {
     Left,
@@ -77,18 +66,18 @@ enum Assoc {
 const RANGE_PRECEDENCE: u8 = 1;
 
 pub(super) fn expr(s: &mut ParserContext) -> Result<Expr> {
-    expr_op(s, OpContext::Precedence(0))
+    expr_op(s, 0)
 }
 
 pub(super) fn expr_with_lhs(s: &mut ParserContext, lhs: Expr) -> Result<Expr> {
-    expr_op_with_lhs(s, OpContext::Precedence(0), lhs)
+    expr_op_with_lhs(s, 0, lhs)
 }
 
-fn expr_op(s: &mut ParserContext, context: OpContext) -> Result<Expr> {
+fn expr_op(s: &mut ParserContext, min_precedence: u8) -> Result<Expr> {
     let lo = s.peek().span.lo;
     let lhs = if let Some(op) = prefix_op(op_name(s)) {
         s.advance();
-        let rhs = expr_op(s, OpContext::Precedence(op.precedence))?;
+        let rhs = expr_op(s, op.precedence)?;
         Expr {
             span: s.span(lo),
             kind: Box::new(ExprKind::UnaryOp(UnaryOpExpr {
@@ -100,14 +89,12 @@ fn expr_op(s: &mut ParserContext, context: OpContext) -> Result<Expr> {
         expr_base(s)?
     };
 
-    expr_op_with_lhs(s, context, lhs)
+    expr_op_with_lhs(s, min_precedence, lhs)
 }
 
-fn expr_op_with_lhs(s: &mut ParserContext, context: OpContext, mut lhs: Expr) -> Result<Expr> {
+fn expr_op_with_lhs(s: &mut ParserContext, min_precedence: u8, mut lhs: Expr) -> Result<Expr> {
     let lo = lhs.span.lo;
 
-    let OpContext::Precedence(min_precedence) = context;
-
     while let Some(op) = infix_op(op_name(s)) {
         if op.precedence < min_precedence {
             break;
@@ -117,7 +104,7 @@ fn expr_op_with_lhs(s: &mut ParserContext, context: OpContext, mut lhs: Expr) ->
         let kind = match op.kind {
             OpKind::Binary(kind, assoc) => {
                 let precedence = next_precedence(op.precedence, assoc);
-                let rhs = expr_op(s, OpContext::Precedence(precedence))?;
+                let rhs = expr_op(s, precedence)?;
                 Box::new(ExprKind::BinaryOp(BinaryOpExpr { op: kind, lhs, rhs }))
             }
             OpKind::Funcall => {

compiler/qsc_qasm3/src/semantic/ast/const_eval.rs

@@ -299,7 +299,6 @@ impl BinaryOpExpr {
                 }
                 Type::Float(..) => {
                     rewrap_lit!((lhs, rhs), (Float(lhs), Float(rhs)), {
-                        // TODO: we need to issue the same lint in Q#.
                         #[allow(clippy::float_cmp)]
                         Bool(lhs == rhs)
                     })
@@ -321,7 +320,6 @@ impl BinaryOpExpr {
                 }
                 Type::Float(..) => {
                     rewrap_lit!((lhs, rhs), (Float(lhs), Float(rhs)), {
-                        // TODO: we need to issue the same lint in Q#.
                         #[allow(clippy::float_cmp)]
                         Bool(lhs != rhs)
                     })
@@ -586,12 +584,11 @@ fn cast_to_int(cast: &Cast, ctx: &mut Lowerer) -> Option<LiteralKind> {
         Type::BitArray(..) => {
             rewrap_lit!(lit, Bitstring(val, _), Int(i64::try_from(val).ok()?))
         }
-        // TODO: UInt Overflowing behavior.
-        //       This is tricky because the inner repersentation
-        //       already is a i64. Therefore, there is nothing to do?
+        // UInt Overflowing behavior.
+        // This is tricky because the inner representation of UInt
+        // is already an i64. Therefore, there is nothing to do.
         Type::Int(..) | Type::UInt(..) => Some(lit),
         Type::Float(..) => rewrap_lit!(lit, Float(val), {
-            // TODO: we need to issue the same lint in Q#.
             #[allow(clippy::cast_possible_truncation)]
             Int(val as i64)
         }),
@@ -616,13 +613,11 @@ fn cast_to_uint(cast: &Cast, ctx: &mut Lowerer) -> Option<LiteralKind> {
         Type::BitArray(..) => {
             rewrap_lit!(lit, Bitstring(val, _), Int(i64::try_from(val).ok()?))
         }
-        // TODO: Int Overflowing behavior.
-        //       This is tricky because the inner representation
-        //       is a i64. Therefore, even we might end with the
-        //       same result anyways. Need to think through this.
+        // UInt Overflowing behavior.
+        // This is tricky because the inner representation of UInt
+        // is already an i64. Therefore, there is nothing to do.
         Type::Int(..) | Type::UInt(..) => Some(lit),
         Type::Float(..) => rewrap_lit!(lit, Float(val), {
-            // TODO: we need to issue the same lint in Q#.
             #[allow(clippy::cast_possible_truncation)]
             Int(val as i64)
         }),
@@ -644,7 +639,6 @@ fn cast_to_float(cast: &Cast, ctx: &mut Lowerer) -> Option<LiteralKind> {
     match &cast.expr.ty {
         Type::Bool(..) => rewrap_lit!(lit, Bool(val), Float(if val { 1.0 } else { 0.0 })),
         Type::Int(..) | Type::UInt(..) => rewrap_lit!(lit, Int(val), {
-            // TODO: we need to issue the same lint in Q#.
             #[allow(clippy::cast_precision_loss)]
             Float(safe_i64_to_f64(val)?)
         }),

compiler/qsc_qasm3/src/semantic/lowerer.rs

@@ -3011,7 +3011,6 @@ impl Lowerer {
         )
     }
 
-    // TODO: which these are parsed as different types, they are effectively the same
     fn lower_index_element(&mut self, index: &syntax::IndexElement) -> semantic::IndexElement {
         match index {
             syntax::IndexElement::DiscreteSet(set) => {