Code review comment for lp:~morphis/aethercast/cleanup-things

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

Looks good, just two comments:

1.- Use of trace macros seems inconsistent. I see ERROR, DEBUG, and then AC_INFO. Please use one convention in a consistent way. Even more, I would prefer to remove duplicate macros in Logger class.

2.- I still see some instances of 'mcs':

src/ac/config.h.in
18:#ifndef MCS_CONFIG_H_
19:#define MCS_CONFIG_H_

src/ac/report/lttng/senderreport.cpp
29: mcs_tracepoint(aethercast_sender, sent_packet, timestamp, size);

src/ac/report/lttng/encoderreport.cpp
29: mcs_tracepoint(aethercast_encoder, started, 0);
33: mcs_tracepoint(aethercast_encoder, stopped, 0);
37: mcs_tracepoint(aethercast_encoder, began_frame, timestamp);
41: mcs_tracepoint(aethercast_encoder, finished_frame, timestamp);
45: mcs_tracepoint(aethercast_encoder, received_input_buffer, timestamp);

src/ac/report/lttng/utils.h
30:#define mcs_tracepoint(c, ...) tracepoint(c, __VA_ARGS__)

src/ac/report/lttng/packetizerreport.cpp
29: mcs_tracepoint(aethercast_packetizer, packetized_frame, timestamp);

src/ac/report/lttng/rendererreport.cpp
29: mcs_tracepoint(aethercast_renderer, began_frame, 0);
33: mcs_tracepoint(aethercast_renderer, finished_frame, timestamp);

src/ac/video/utils.cpp
39:} // mcs

src/ac/video/utils.h
31:} // mcs

src/ac/logger.h
103:// Log returns the mcs-wide configured logger instance.
106:// SetLog installs the given logger as mcs-wide default logger.

tests/CMakeLists.txt
13: ${CMAKE_SOURCE_DIR}/src/mcs

src/gdbus/CMakeLists.txt
3:file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/mcs)

tests/ac/CMakeLists.txt
9: ${CMAKE_SOURCE_DIR}/src/mcs
10: ${CMAKE_SOURCE_DIR}/src/mcs/w11t/gdhcp

tests/ac/integration_tests/config.h.in
18:#ifndef MCS_TESTS_INTEGRATION_CONFIG_H_
19:#define MCS_TESTS_INTEGRATION_CONFIG_H_
27:} // namespace mcs

tests/common/dbusfixture.cpp
100: config_path += "/mcs-dbus-system-";

review: Needs Fixing

« Back to merge proposal