1

I have a routine in the main.cpp where user will specify which mode to execute in the program. Once the mode is specified, the corresponding block will be executed - first downcast the parent solver to the child solver and call the associated solve method in the child class.

    std::unique_ptr<SudokuSolver> solver;
    if (mode == MODES::SEQUENTIAL_BACKTRACKING) 
    {
        solver = std::make_unique<SudokuSolver_SequentialBacktracking>();
        SudokuSolver_SequentialBacktracking* child_solver = dynamic_cast<SudokuSolver_SequentialBacktracking*>(solver.get());
        child_solver->solve(board);
    } 
    else if (mode == MODES::SEQUENTIAL_BRUTEFORCE)
    {
        solver = std::make_unique<SudokuSolver_SequentialBruteForce>();
        SudokuSolver_SequentialBruteForce* child_solver = dynamic_cast<SudokuSolver_SequentialBruteForce*>(solver.get());
        child_solver->solve(board);
    }   
    else if (mode == MODES::PARALLEL_BRUTEFORCE)
    {
        int NUM_THREADS = (argc >= 5) ? std::stoi(argv[4]) : 2;
        omp_set_num_threads(NUM_THREADS);
        
        #pragma omp parallel
        {
            #pragma omp single nowait
            {
                solver = std::make_unique<SudokuSolver_ParallelBruteForce>();
                SudokuSolver_ParallelBruteForce* child_solver = dynamic_cast<SudokuSolver_ParallelBruteForce*>(solver.get());
                child_solver->solve(board);
            }
        }
    }
    else if (mode == MODES::SEQUENTIAL_DANCINGLINKS)
    {
        solver = std::make_unique<SudokuSolver_SequentialDLX>(board);
        SudokuSolver_SequentialDLX* child_solver = dynamic_cast<SudokuSolver_SequentialDLX*>(solver.get());
        child_solver->solve();
    }
    else if (mode == MODES::PARALLEL_DANCINGLINKS)
    {
        int NUM_THREADS = (argc >= 5) ? std::stoi(argv[4]) : 2;
        omp_set_num_threads(NUM_THREADS);

        #pragma omp parallel
        {
            #pragma omp single
            {
                solver = std::make_unique<SudokuSolver_ParallelDLX>(board);
                SudokuSolver_ParallelDLX* child_solver = dynamic_cast<SudokuSolver_ParallelDLX*>(solver.get());
                child_solver->solve();
            }
        }
    }

I found it's kinda duplicates of code so I want to templatize them to something like:

template <typename T>
void downCastandSolve(std::unique_ptr<SudokuSolver> solver, SudokuBoard& board)
{
    solver = std::make_unique<T>(board);
    T* child_solver = dynamic_cast<T*>(solver.get());
    child_solver->solve();
}

However, I got the following error message:

error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = SudokuSolver; _Dp = std::default_delete<SudokuSolver>]’

Not sure how to properly templatize the portions of code. Hope someone could help!

JeJo
  • 30,635
  • 6
  • 49
  • 88
livemyaerodream
  • 890
  • 6
  • 19
  • 2
    It look like you could replace the need of `dynamic_cast` by making `solve` function virtual... Also, it seems that `std::unique_ptr solver` is not really useful as it is never used and only make remaining code more complicated by adding extra casts... – Phil1970 Aug 10 '21 at 21:11
  • 2
    Does `solver` get used subsequent to this branching code? Because as @Phil1970 says, the code without it is very simple: `T(board).solve();` – Ben Voigt Aug 10 '21 at 21:30
  • @Phil1970 @Ben Voigt Yes, ```solver``` is used after the branching code to access the solution stored in the ```SudokuSolver``` class by ```solver->get_solution()```. That's why I need to downcast here. But I hope there is better suggestion for my design... – livemyaerodream Aug 10 '21 at 21:38
  • 1
    Obviously `auto child_solver = std::make_unique(board); child_solver->solve(); solver = std::move(child_solver);` would avoid the useless cast without any other changes. ` – Phil1970 Aug 10 '21 at 21:44
  • Parallel code also seems suspicious as there are no loop or way to combine results... – Phil1970 Aug 10 '21 at 21:48
  • @Phil1970 The parallel code is implemented in the associated ```solve``` method in the child class. – livemyaerodream Aug 10 '21 at 22:00

2 Answers2

5

You would have much clearer code if solve function was virtual.

Also, either the constructor or the solve method should receive the board reference but not both. Having a single way make the code simpler to understand as each algorithm works the same way.

std::unique_ptr<SudokuSolver> CreateSolver(MODES mode)
{
    switch (mode)
    {
        case MODES::SEQUENTIAL_BACKTRACKING:
            return std::make_unique<SudokuSolver_SequentialBacktracking>();
        ...
    }
}

Then assuming solve is virtual, the code resume to:

auto solver = CreateSolver(mode);
solver->solve(board);
solver->get_solution();

Any parallel stuff should be hidden inside the solve function when appropriate.

By writing code like that, it is much easier to read and you don't need an extra template function to share code as you avoid duplication from the start.

Phil1970
  • 2,605
  • 2
  • 14
  • 15
4

std::unique_ptrs are not copyable. You need to take it by reference:

void downCastandSolve(std::unique_ptr<SudokuSolver>& solver, SudokuBoard& board)
//                                                 ^
Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108