LINUX.ORG.RU

Покритикуйте код

 , ,


0

1

Написал простенький класс для парсинга арифметических выражений с поддержкой регулярок и шаблонов. Если кому не лень, подскажите, что можно исправить/улучшить/переписать. Основной файл - expression_parser.hpp

https://github.com/selat/expression-parser


diff --git a/CMakeLists.txt b/CMakeLists.txt
index d051f5d..175fa2f 100755
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -4,7 +4,6 @@ project(expression-parser)
 
 set(CMAKE_VERBOSE_MAKEFILE OFF)
 set(CMAKE_BUILD_TYPE Debug)
-set(EXECUTABLE_OUTPUT_PATH "${PROJECT_SOURCE_DIR}")
 
 set(${PROJECT_NAME}_VERSION_MAJOR 0)
 set(${PROJECT_NAME}_VERSION_MINOR 0)
@@ -13,7 +12,7 @@ set(${PROJECT_NAME}_VERSION "${${PROJECT_NAME}_VERSION_MAJOR}.${${PROJECT_NAME}_
 message(STATUS "${PROJECT_NAME} ${${PROJECT_NAME}_VERSION}")
 
 set(CMAKE_CXX_COMPILER "clang++")
-set(CMAKE_CXX_FLAGS "-std=c++11 -stdlib=libc++ -Wall")
+set(CMAKE_CXX_FLAGS "-std=c++11 -Wall")
 set(CMAKE_CXX_FLAGS_RELEASE "-O2")
 set(CMAKE_CXX_FLAGS_DEBUG "-g -O0 -fno-inline")
Deleted
()
Ответ на: комментарий от Deleted
enum LexemeType {FUNCTION, PARENTHESIS, OPERATOR, ARGUMENT, UNKNOWN};

enum class

Cell <T>* parse();

std::shared_ptr? И вообще как-то много указателей используется для std=c++11. Ты уверен, что у тебя всегда всё удаляется как надо? Например, при выпадении исключений.

И что будет, если скопировать инстанс, например, класса ExpressionParser? Может таки сделать его некопируемым?

P.S. В детали не вникал.

Deleted
()
Ответ на: комментарий от Deleted

Большое спасибо за ценные указания!

std::shared_ptr? И вообще как-то много указателей используется для std=c++11. Ты уверен, что у тебя всегда всё удаляется как надо? Например, при выпадении исключений.

Я с smart pointers не очень хорошо знаком, поэтому редко их использую. Похоже, что сейчас самое время нормально разобраться с ними.

И что будет, если скопировать инстанс, например, класса ExpressionParser? Может таки сделать его некопируемым?

Да, с копированием действительно есть проблема. Получается, что запрет копирования - это нормальная и довольно часто используемая конструкция?

Selat
() автор топика
Ответ на: комментарий от Selat

Получается, что запрет копирования - это нормальная и довольно часто используемая конструкция?

Тут как бы всего два варианта: либо запретить копирование, либо удостовериться, что оно работает нормально и ничего не ломает. Если копирование в принципе не имеет смысла для какого-то класса, то почему бы его не запретить вообще?

Deleted
()

Вникать лень, так что просто покритикую код, можешь считать придирками и игнорировать, если хочешь:

1. Закомментированный код - отстой. Да и разве не будет предупреждения на неиспользуемую функцию testSpeed? Кстати, тесты завести было бы неплохо.

2. Хорошая практика, делать конструкторы с одним параметром explicit (если нет оснований делать иначе).

3. Неплохо бы добавить move конструктор и оператор присваивания.

4. Зачем в заголовочном файле m_whitespaces, m_operators, m_functions? Я бы перенёс в cpp (в безымянный неймспейс).

5. throw() надо заменить на noexcep.

6. Использовать override.

7. Про смартпоинтеры уже сказали.

8. Вывод посреди логики, я бы как-то разделить постарался.

9. return (m_root == e.m_root) || (*m_root == *e.m_root); Это что? Один из указателей не может оказаться nullptr? Если может, то будет плохо.

10. «Инклюд гарды», конечно, дело вкуса, но pragma once понимают все современные компиляторы.

11. Местами include тебя странно сгруппированы. В основном, iostream особняком стоит, но в expression.cpp - нет.

12. Авто, вроде, используешь, но не везде: Functions<int>::const_iterator res = m_operators.end();

DarkEld3r ★★★★★
()
Последнее исправление: DarkEld3r (всего исправлений: 2)
-set(CMAKE_CXX_COMPILER "clang++")
-set(CMAKE_CXX_FLAGS "-std=c++11 -Wall")
-set(CMAKE_CXX_FLAGS_RELEASE "-O2")
-set(CMAKE_CXX_FLAGS_DEBUG "-g -O0 -fno-inline")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall")

Хоть я и использую исключительно clang, хардкодить компилятор нельзя. Также нельзя переопределять флаги, только дополнять. В _RELEASE/_DEBUG вообще лезть не надо - как оптимизирует и отлаживает каждый конкретный пользователь проекта - не вашего ума дело.

slovazap ★★★★★
()
Ответ на: комментарий от slovazap

категорически не верно.

set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic -std=gnu++11")
set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -g")
set (CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -O3")
set (CMAKE_CXX_FLAGS_MINSIZEREL "${CMAKE_CXX_FLAGS_MINSIZEREL} -Os")
set (CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} -g -O3")
x0r ★★★★★
()
Последнее исправление: x0r (всего исправлений: 1)
Ответ на: комментарий от x0r

Слово «категорически» вы использовали только чтобы выглядеть важнее, потому что по сути просто накидали флагов по своему вкусу. Довольно, надо заметить, безграмотно, поскольку то что вы «добавляете» к флагам повторяет их значение по умолчанию. Ну а за gnu++11 надо вообще бить по рукам.

slovazap ★★★★★
()
Ответ на: комментарий от slovazap

речь не о значениях флагов (я копипастнул из своего проекта не глядя, что там у ОПа), речь об CMAKE_CXX_FLAGS_*.

x0r ★★★★★
()
Ответ на: комментарий от x0r

Не распарсил. Повторяю, ваши строки с CMAKE_CXX_FLAGS_* бесполезны и не нужны, потому что ничего не меняют. И выдают незнание системы сборки.

slovazap ★★★★★
()

Да, кстати, set(CMAKE_BUILD_TYPE Debug) тоже нужно убрать.

slovazap ★★★★★
()

с поддержкой регулярок и шаблонов

Я не понял, каким макаром это относится к арифметическим выражениям? Что вводить то? В ридми написано, что пример использования в main.cpp - там конечно есть то, как строки передавать программе, но что вводить - непонятно.

hlebushek ★★
()

Покритикуйте код

Твой код — говно.

ee1337a
()

Кстати, а какая асимптотика у regex_search? Просто алгоритм парсинга вроде линейный, но по времени работы очень сильно похож на квадрат.

Selat
() автор топика
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.